[PATCH] D54630: Move detection of libc++ include dirs to Driver on MacOS

arphaman added a comment. I don't think we want to move the logic to add a libc++ path to the driver. `-cc1` with `-resource-dir` and `-stdlib=libc++` should still work as before. In this case the previous patch is better, except it should not set `InstalledDir` except when needed (e.g. for too

[PATCH] D49736: [Basic] Emit warning flag suggestion only in case there's existing flag *similar* to the unknown one

arphaman added a comment. Thanks for working on this, some minor comments: Comment at: Basic/DiagnosticIDs.cpp:612 // Two matches with the same distance, don't prefer one over the other. - Best = ""; + NearestOption = ""; } else if (Distance < BestDistance

[PATCH] D54630: Move detection of libc++ include dirs to Driver on MacOS

arphaman added a comment. In D54630#1302566 , @ilya-biryukov wrote: > In D54630#1301283 , @arphaman wrote: > > > I don't think we want to move the logic to add a libc++ path to the driver. > > `-cc1` with `-resour

[PATCH] D54799: [clangd][WIP] textDocument/SymbolInfo method

arphaman added a comment. LGTM, we can optimize later. It looks like you also moved `SymbolID` to a new file, this should be an NFC precommit. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54799/new/ https://reviews.llvm.org/D54799 ___ cfe

[PATCH] D54630: Move detection of libc++ include dirs to Driver on MacOS

arphaman added a comment. So far everything looks fine, but I have to rerun a couple of things due to infrastructural issues. I should have the final results next Monday. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54630/new/ https://reviews.llvm.org/D54630

[PATCH] D54630: Move detection of libc++ include dirs to Driver on MacOS

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54630/new/ https://reviews.llvm.org/D54630 ___ cfe-commit

[PATCH] D54630: Move detection of libc++ include dirs to Driver on MacOS

arphaman added a comment. Please mention this change in the release notes though. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54630/new/ https://reviews.llvm.org/D54630 ___ cfe-commits mailing list cfe-commits@list

[PATCH] D55322: Mention changes to libc++ include dir lookup in release notes.

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55322/new/ https://reviews.llvm.org/D55322 ___ cfe-commi

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

arphaman requested changes to this revision. arphaman added a comment. Swift uses `-Xclang` to pass in build settings to its own build and to pass in custom options through its Clang importer that we intentionally don't want to expose to Clang's users. We don't want to warn for those uses for su

[PATCH] D55455: Remove stat cache chaining as it's no longer needed after PTH support has been removed

arphaman created this revision. arphaman added reviewers: erichkeane, dexonsmith, vsapsai, Bigcheese, ilya-biryukov. Herald added a subscriber: jkorous. Stat cache chaining was implemented for a StatListener in the PTH writer so that it could write out the stat information to PTH. r348266 remove

[PATCH] D55463: Introduce a source minimizer that reduces source to directives that might affect the dependency list for a compilation

arphaman created this revision. arphaman added reviewers: dexonsmith, Bigcheese, dblaikie, vsapsai, sammccall, rsmith, bruno. Herald added subscribers: jkorous, mgorny. This patch introduces a dependency directives source minimizer to clang that minimizes header and source files to the minimum n

[PATCH] D55455: Remove stat cache chaining as it's no longer needed after PTH support has been removed

arphaman added a comment. In D55455#1323836 , @dexonsmith wrote: > This patch LGTM, but before committing I suggest sending an RFC to cfe-dev > and waiting a few days. I posted to `cfe-dev` to check if there are any objections. Repository: rC Clang

[PATCH] D55544: Warning: objc-encodings-larger-than=

arphaman added a comment. Thanks for working on this! Could you please post a patch with full context (git diff -U)? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55544/new/ https://reviews.llvm.org/D55544 ___ c

[PATCH] D55463: Introduce a source minimizer that reduces source to directives that might affect the dependency list for a compilation

arphaman updated this revision to Diff 193214. arphaman added a comment. Herald added a subscriber: jdoerfert. Herald added a project: clang. Rebased patch, added a `#warning/error` fix from @dexonsmith. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55463/new/ ht

[PATCH] D55463: Introduce a source minimizer that reduces source to directives that might affect the dependency list for a compilation

arphaman added a comment. @Bigcheese Please take a look. I'll post the basic tool that runs the preprocessor on regular vs minimized sources by Wed. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55463/new/ https://reviews.llvm.org/D55463 _

[PATCH] D56924: Special case ObjCPropertyDecl for printing

arphaman added a comment. Sorry, I missed the pings. It LGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56924/new/ https://reviews.llvm.org/D56924 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://l

[PATCH] D60233: [clang-scan-deps] initial outline of the tool that runs preprocessor to find dependencies over a JSON compilation database

arphaman created this revision. arphaman added a reviewer: Bigcheese. Herald added subscribers: jfb, dexonsmith, jkorous, mgorny. Herald added a project: clang. This patch introduces an outline for the `clang-scan-deps` tool that will be used to implement fast dependency discovery phase using imp

[PATCH] D55463: Introduce a source minimizer that reduces source to directives that might affect the dependency list for a compilation

arphaman added a comment. @Bigcheese Initial tool posted to https://reviews.llvm.org/D60233 . Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55463/new/ https://reviews.llvm.org/D55463 ___ cfe-commits mailing list cfe

[PATCH] D55463: Introduce a source minimizer that reduces source to directives that might affect the dependency list for a compilation

arphaman updated this revision to Diff 194331. arphaman added a comment. Updated to the new LLVM license comments. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55463/new/ https://reviews.llvm.org/D55463 Files: include/clang/Basic/DiagnosticFrontendKinds.td

[PATCH] D60432: [clang][ASTContext] Simplify caching for declaration-related comments

arphaman added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:372 D = adjustDeclToTemplate(D); + const Decl* CanonicalDecl = D->getCanonicalDecl(); Why are we now checking for the canonical declaration instead of `D` as before? Repository: rC

[PATCH] D60735: [FileSystemStatCache] Return std::error_code from stat cache methods

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. LGTM, please mark it as NFC Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60735/new/ https://reviews.llvm.org/D60735

[PATCH] D60786: [FileSystemStatCache] Update test for new FileSystemStatCache API

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. Herald added a subscriber: ormris. LG Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60786/new/ https://reviews.llvm.org/D60786 __

[PATCH] D61758: [driver][xray] fix the macOS support checker by supporting -macos triple in addition to -darwin

arphaman created this revision. arphaman added a reviewer: dberris. Herald added subscribers: dexonsmith, jkorous. Herald added a project: clang. The previous check incorrectly checked for macOS support by allowing `-darwin` triples only, and `-macos` triple was not supported. Repository: rG

[PATCH] D57345: Make clang/test/Index/pch-from-libclang.c pass in more places

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57345/new/ https://reviews.llvm.org/D57345 ___ cfe-commits mailing list cfe-commi

[PATCH] D57532: [Index] Make sure c-index-test finds libc++ on Mac

arphaman requested changes to this revision. arphaman added a comment. This revision now requires changes to proceed. The real problem is that `clang_parseTranslationUnit2` is broken, so this just fixes one client, `c-index-test`. All other macOS clients that call that function will still be una

[PATCH] D57532: [Index] Make sure c-index-test finds libc++ on Mac

arphaman added a comment. In D57532#1380383 , @ilya-biryukov wrote: > @arphaman, thanks for pointing this out. It's definitely nice to change stuff > only in one place. > There are two issues still pending: > > - `index-file.cu` still fails, I haven't l

[PATCH] D57712: [SemaObjC] Don't infer the availabilty of messages to +new from -init if the receiver has Class type

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. LGTM, Thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57712/new/ https://reviews.llvm.org/D57712 ___ c

[PATCH] D57532: [Index] Make sure c-index-test finds libc++ on Mac

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57532/new/ https://reviews.llvm.org/D57532 __

[PATCH] D58213: [FileManager] fillRealPathName even if we aren't opening the file

arphaman added inline comments. Comment at: lib/Basic/FileManager.cpp:271 + } else if (!openFile) { +// Since we didn't return early after getStatValue() call the file exists. +fillRealPathName(&UFE, InterndFileName); I don't really understand what this

[PATCH] D58213: [FileManager] fillRealPathName even if we aren't opening the file

arphaman added inline comments. Comment at: lib/Basic/FileManager.cpp:271 + } else if (!openFile) { +// Since we didn't return early after getStatValue() call the file exists. +fillRealPathName(&UFE, InterndFileName); jkorous wrote: > arphaman wrote: > >

[PATCH] D58213: [FileManager] fillRealPathName even if we aren't opening the file

arphaman added inline comments. Comment at: unittests/Basic/FileManagerTest.cpp:349 +// rdar://problem/47536127 +TEST_F(FileManagerTest, getFileDontOpenRealPath) { jkorous wrote: > arphaman wrote: > > Please leave this comment out > Sure, no problem. > > Just

[PATCH] D58213: [FileManager] fillRealPathName even if we aren't opening the file

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58213/new/ https://reviews.llvm.org/D58213 ___ cfe-commits mailing list cfe-commi

[PATCH] D58559: emit '(assertions enabled)' in the version string for a build of clang with assertions enabled

arphaman created this revision. arphaman added a reviewer: dexonsmith. Herald added a subscriber: jkorous. Herald added a project: clang. This patch adds an additional '(assertions enabled)' in the version string for a build of clang with assertions enabled. This is a useful way to differentiate

[PATCH] D58559: emit '(assertions enabled)' in the version string for a build of clang with assertions enabled

arphaman updated this revision to Diff 188023. arphaman marked an inline comment as done. arphaman added a comment. Use `"(+asserts)"` as suggested. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58559/new/ https://reviews.llvm.org/D58559 Files: lib/Basic/Versi

[PATCH] D56924: Handle ObjCCategoryDecl class extensions for print

arphaman added a comment. Please add a test that covers the '(class extension)' output as well. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56924/new/ https://reviews.llvm.org/D56924 ___ cfe-commits mailing list cf

[PATCH] D58559: emit '(assertions enabled)' in the version string for a build of clang with assertions enabled

arphaman added a comment. In D58559#1407742 , @dexonsmith wrote: > I like this. Can you start a discussion on cfe-dev (if you haven't already)? > This is user-visible so I want to be sure other vendors are happy with this; > if not, we can hide it behi

[PATCH] D36075: [refactor] Initial support for refactoring action rules

arphaman marked 5 inline comments as done. arphaman added inline comments. Comment at: include/clang/Tooling/Refactoring/RefactoringActionRules.h:33 +/// +/// - requiredSelection: The refactoring function won't be invoked unless the +/// given selection req

[PATCH] D36075: [refactor] Initial support for refactoring action rules

arphaman updated this revision to Diff 112678. arphaman added a comment. - Get rid of `RefactoringResult` in favour of different rule types. - Rename `RefactoringOperation` to `RefactoringRuleContext`. - Address a couple more comments. Repository: rL LLVM https://reviews.llvm.org/D36075 Fil

[PATCH] D37001: [clang-diff] Use data collectors for node comparison

arphaman added a comment. Can you remove `getNodeValue` now or do you still need it? https://reviews.llvm.org/D37001 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37005: Add include/clang/Tooling/ASTDiff/ASTPatch.h

arphaman added a comment. I don't think AST manipulation is the right way to do patching. You've already hit the limitations here in this patch where you can't really remove a statement unless its parent is a `CompoundStmt`. I think the right way to do AST patching is to take a 3rd AST(lets cal

[PATCH] D36790: [ObjC] Messages to 'self' in class methods should use class method dispatch to avoid multiple method ambiguities

arphaman added a comment. @doug.gregor Ping Repository: rL LLVM https://reviews.llvm.org/D36790 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37004: [clang-diff] Fix the html output for CXXOperatorCallExpr

arphaman added inline comments. Comment at: tools/clang-diff/ClangDiff.cpp:319 + "A Binary operator is supposed to have two arguments."); +for (int I : {1, 0, 2}) + Offset = printHtmlForNode(OS, Diff, Tree, IsLeft, Children[I], Offset); johanne

[PATCH] D36969: [Basic] Add a DiagnosticError llvm::ErrorInfo subclass

This revision was automatically updated to reflect the committed changes. Closed by commit rL311778: [Basic] Add a DiagnosticError llvm::ErrorInfo subclass (authored by arphaman). Changed prior to commit: https://reviews.llvm.org/D36969?vs=112327&id=112700#toc Repository: rL LLVM https://re

[PATCH] D37104: [libc++] PR34298: Change std::function constructor and move assignment operator SFINAE checks to allow std::function with an incomplete return type

arphaman added a comment. Not at all, go ahead Repository: rL LLVM https://reviews.llvm.org/D37104 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37104: [libc++] PR34298: Change std::function constructor and move assignment operator SFINAE checks to allow std::function with an incomplete return type

arphaman added a comment. FYI, I'd like to see this merged into LLVM5 if it's still possible, but it's not super critical Repository: rL LLVM https://reviews.llvm.org/D37104 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.ll

[PATCH] D36075: [refactor] Initial support for refactoring action rules

arphaman added a comment. Thanks, I'll start working on the documentation patch and will split follow-up `clang-refactor` patch into one or two parts today. Comment at: include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h:33 +public: + virtual Expected> + per

[PATCH] D37005: [clang-diff] Initial implementation of patching

arphaman added inline comments. Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:658 + SourceRange Range; + if (auto *Arg = N.ASTNode.get()) +Range = TemplateArgumentLocations.at(&N - &Nodes[0]); You can drop the `auto *Arg` since Arg is unused. ==

[PATCH] D37200: [AST] Traverse CXXOperatorCallExpr in LexicallyOrderedRecursiveASTVisitor

arphaman added inline comments. Comment at: include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h:69 + template + struct has_same_member_pointer_type + : std::true_type {}; Why exactly is this template struct needed? Comment at: inclu

[PATCH] D37200: [AST] Traverse CXXOperatorCallExpr in LexicallyOrderedRecursiveASTVisitor

arphaman added a comment. Ah I see, then you should something completely different (forget the header). Please extract this code into a protected function in RecursiveASTVisitor: for (Stmt *SubStmt : Children) if (!TRAVERSE_STMT_BASE(Stmt, Stmt, SubStmt, Queue)) return false; And re

[PATCH] D36075: [refactor] Initial support for refactoring action rules

This revision was automatically updated to reflect the committed changes. Closed by commit rL311884: [refactor] initial support for refactoring action rules (authored by arphaman). Changed prior to commit: https://reviews.llvm.org/D36075?vs=112678&id=112879#toc Repository: rL LLVM https://r

[PATCH] D37210: [refactor] add a refactoring action rule that returns symbol occurrences

arphaman created this revision. This patch adds a second kind of refactoring action rule that produces symbol occurrences. It will be used by the updated `clang-refactor` patch at https://reviews.llvm.org/D36574. Repository: rL LLVM https://reviews.llvm.org/D37210 Files: include/clang/To

[PATCH] D37200: [AST] Traverse CXXOperatorCallExpr in LexicallyOrderedRecursiveASTVisitor

arphaman added a comment. This works as well I think https://reviews.llvm.org/D37200 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37200: [AST] Traverse CXXOperatorCallExpr in LexicallyOrderedRecursiveASTVisitor

arphaman added inline comments. Comment at: include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h:63 using BaseType = RecursiveASTVisitor; + using typename BaseType::DataRecursionQueue; Do you still need the using here? https://reviews.llvm.org/D37200

[PATCH] D37200: [AST] Traverse CXXOperatorCallExpr in LexicallyOrderedRecursiveASTVisitor

arphaman added inline comments. Comment at: include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h:149 + auto Offset = [&](Stmt *S) { return SM.getFileOffset(S->getLocStart()); }; + Swap = Offset(Children[0]) > Offset(Children[1]); + break; For `

[PATCH] D37200: [AST] Traverse CXXOperatorCallExpr in LexicallyOrderedRecursiveASTVisitor

arphaman added a comment. The fact `TraverseCXXOperatorCallExpr` can't call its super `TraverseCXXOperatorCallExpr` makes the current solution kind of broken. The super implementation of `TraverseCXXOperatorCallExpr` is responsible for dispatch to WalkUp##STMT functions which actually call `Vi

[PATCH] D36075: [refactor] Initial support for refactoring action rules

arphaman added inline comments. Comment at: cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRule.h:23-37 +/// A common refactoring action rule interface. +class RefactoringActionRule { +public: + enum RuleKind { SourceChangeRefactoringRuleKind }; + + RuleKind getR

[PATCH] D37200: [AST] Traverse CXXOperatorCallExpr in LexicallyOrderedRecursiveASTVisitor

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. Lgtm with two requests below Comment at: include/clang/AST/RecursiveASTVisitor.h:354 +protected: + bool TraverseStmtBase(Stmt *S, DataRecursionQueue *Queue) { +retur

[PATCH] D37005: [clang-diff] Initial implementation of patching

arphaman added inline comments. Comment at: include/clang/Tooling/ASTDiff/ASTPatch.h:1 +//===- ASTDiff.h - Structural patching based on ASTDiff ---*- C++ -*- -===// +// Please update the file name in the comment Comment at: lib/Tooling/AST

[PATCH] D37005: [clang-diff] Initial implementation of patching

arphaman added inline comments. Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:73 public: + /// Empty (invalid) SyntaxTree. + SyntaxTree(); Why do you need to create an empty tree? What about using llvm::Optional instead? Comment at: i

[PATCH] D37001: [clang-diff] Use data collectors for node comparison

arphaman added inline comments. Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:537 + +#include "../../AST/DeclDataCollectors.inc" + I didn't realize that you're including files from within `lib`. That's not ideal. You should add a pre-commit that moves the *.inc fi

[PATCH] D37210: [refactor] add a refactoring action rule that returns symbol occurrences

arphaman added inline comments. Comment at: include/clang/Tooling/Refactoring/RefactoringActionRule.h:29 +SourceChangeRefactoringRuleKind, +FindSymbolOccurrencesRefactoringRuleKind + }; hokein wrote: > I might miss some context here. As per your comment

[PATCH] D37291: [refactor] Use a RefactoringResultConsumer instead of tagged refactoring rule classes

arphaman created this revision. Herald added a reviewer: JonasToth. This patch changes the way that the refactoring results are produced. Instead of using different `RefactoringActionRule` subclasses for each result type, we now use a single `RefactoringResultConsumer`. This was suggested by Man

[PATCH] D37291: [refactor] Use a RefactoringResultConsumer instead of tagged refactoring rule classes

arphaman updated this revision to Diff 113222. arphaman added a comment. Fixed a comment Repository: rL LLVM https://reviews.llvm.org/D37291 Files: include/clang/Tooling/Refactoring/RefactoringActionRule.h include/clang/Tooling/Refactoring/RefactoringActionRules.h include/clang/Tooling

[PATCH] D37291: [refactor] Use a RefactoringResultConsumer instead of tagged refactoring rule classes

arphaman updated this revision to Diff 113223. arphaman added a comment. Use `std::move` Repository: rL LLVM https://reviews.llvm.org/D37291 Files: include/clang/Tooling/Refactoring/RefactoringActionRule.h include/clang/Tooling/Refactoring/RefactoringActionRules.h include/clang/Tooling

[PATCH] D37210: [refactor] add a refactoring action rule that returns symbol occurrences

arphaman updated this revision to Diff 113224. arphaman marked an inline comment as done. arphaman added a comment. Herald added a reviewer: JonasToth. This revision now requires review to proceed. Rebase on top of https://reviews.llvm.org/D37291 Repository: rL LLVM https://reviews.llvm.org/D

[PATCH] D37291: [refactor] Use a RefactoringResultConsumer instead of tagged refactoring rule classes

arphaman added inline comments. Comment at: include/clang/Tooling/Refactoring/RefactoringResultConsumer.h:39 + /// Handles the source replacements that are produced by a refactoring action. + virtual void handle(AtomicChanges SourceReplacements) = 0; +}; ioeri

[PATCH] D37291: [refactor] Use a RefactoringResultConsumer instead of tagged refactoring rule classes

arphaman added inline comments. Comment at: include/clang/Tooling/Refactoring/RefactoringResultConsumer.h:39 + /// Handles the source replacements that are produced by a refactoring action. + virtual void handle(AtomicChanges SourceReplacements) = 0; +}; ioeri

[PATCH] D37291: [refactor] Use a RefactoringResultConsumer instead of tagged refactoring rule classes

arphaman updated this revision to Diff 113397. arphaman marked an inline comment as done. arphaman added a comment. - Add default implementation for `handle()` - Merge two error handling functions into one https://reviews.llvm.org/D37291 Files: include/clang/Tooling/Refactoring/RefactoringAct

[PATCH] D37291: [refactor] Use a RefactoringResultConsumer instead of tagged refactoring rule classes

arphaman added inline comments. Comment at: unittests/Tooling/RefactoringActionRulesTest.cpp:39 + class Consumer final : public RefactoringResultConsumer { +void handleInitiationFailure() { + Result = Expected>(None); ioeric wrote: > Can we probably hav

[PATCH] D37341: [Sema] Fix an assert-on-invalid by avoiding function template specialisation deduction for invalid functions with fabricated template arguments

arphaman created this revision. This patch fixes an an assertion that previously occurred for the following sample: template struct SourceSelectionRequirement { template OutputT evaluateSelectionRequirement(InputT &&Value) { } }; template OutputT SourceSelectionRequ

[PATCH] D37291: [refactor] Use a RefactoringResultConsumer instead of tagged refactoring rule classes

arphaman added inline comments. Comment at: include/clang/Tooling/Refactoring/RefactoringResultConsumer.h:30 + /// the source selection has no overlap at all with any relevant AST nodes. + virtual void handleInitiationFailure() = 0; + klimek wrote: > ioeric wro

[PATCH] D37291: [refactor] Use a RefactoringResultConsumer instead of tagged refactoring rule classes

arphaman added a comment. In https://reviews.llvm.org/D37291#857727, @klimek wrote: > Thanks! I think this makes the code easier to understand. Now my remaining > question is why the ResultType is templates vs. also being an interface > (sorry for being late, was out on vacation the past 2 week

[PATCH] D37291: [refactor] Use a RefactoringResultConsumer instead of tagged refactoring rule classes

This revision was automatically updated to reflect the committed changes. Closed by commit rL312316: [refactor] Use a RefactoringResultConsumer instead of tagged refactoring (authored by arphaman). Changed prior to commit: https://reviews.llvm.org/D37291?vs=113397&id=113520#toc Repository: r

[PATCH] D37210: [refactor] add a refactoring action rule that returns symbol occurrences

arphaman updated this revision to Diff 113521. arphaman added a comment. Rebase on ToT Repository: rL LLVM https://reviews.llvm.org/D37210 Files: include/clang/Tooling/Refactoring/RefactoringResultConsumer.h unittests/Tooling/RefactoringActionRulesTest.cpp Index: unittests/Tooling/Refa

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

arphaman updated this revision to Diff 113533. arphaman added a comment. Rebase on top of trunk and https://reviews.llvm.org/D37210. Ping. Repository: rL LLVM https://reviews.llvm.org/D36574 Files: include/clang/Tooling/Refactoring/RefactoringAction.h include/clang/Tooling/Refactoring/R

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

arphaman added a comment. In https://reviews.llvm.org/D36574#858763, @klimek wrote: > One of my main concerns is still that I don't see the need for all the > template magic yet :) Why doesn't everybody use the RefactoringResult we > define here? This refactoring result is only really useful

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

arphaman added inline comments. Comment at: test/Refactor/LocalRename/Field.cpp:4 +class Baz { + int /*range=*/Foo; // CHECK: symbol [[@LINE]]:17 -> [[@LINE]]:20 +public: klimek wrote: > Does this just test the selection? No, this is the moved `clang-rename/Fiel

[PATCH] D37383: [AST] Add TableGen for StmtDataCollectors

arphaman added a comment. TableGen is great for structured data, but we shouldn't use it just to embed code in it. What was the issue you've had with the build when the inc file was in include? https://reviews.llvm.org/D37383 ___ cfe-commits maili

[PATCH] D36998: [AST] Traverse templates in LexicallyOrderedRecursiveASTVisitor

arphaman added inline comments. Comment at: include/clang/AST/RecursiveASTVisitor.h:507-508 // Traverses template parameter lists of either a DeclaratorDecl or TagDecl. template bool TraverseDeclTemplateParameterLists(T *D); I don't think you need to

[PATCH] D37383: [AST] Add TableGen for StmtDataCollectors

arphaman added a comment. In https://reviews.llvm.org/D37383#858905, @teemperor wrote: > @arphaman I suggested tablegen in https://reviews.llvm.org/D36664 because I > remembered we had some CMake sanity check about not having an *.inc in our > include dir: > https://github.com/llvm-mirror/clan

[PATCH] D37383: [AST] Add TableGen for StmtDataCollectors

arphaman accepted this revision. arphaman added a comment. `def`s could work, but I think that we should stick with TableGen. As you've said, we'd be able to introduce some sort of structure instead of just code in the future which will be better (Ideally we'd try to do it now, but given the fa

[PATCH] D37383: [AST] Add TableGen for StmtDataCollectors

arphaman added a comment. LGTM with one inline comment below: Comment at: utils/TableGen/TableGen.cpp:151 clEnumValN(GenOptDocs, "gen-opt-docs", "Generate option documentation"), +clEnumValN(GenDataCollectors, "gen-clang-data-collectors", "Generate data colle

[PATCH] D36998: [AST] Traverse templates in LexicallyOrderedRecursiveASTVisitor

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D36998 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/c

[PATCH] D37390: [diagtool] Change default tree behavior to print only flags

arphaman added a comment. The diagtool tests should go to `test/Misc`, and there's an existing `diagtool tree` test there that has to be updated as well. Repository: rL LLVM https://reviews.llvm.org/D37390 ___ cfe-commits mailing list cfe-commit

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

arphaman updated this revision to Diff 113736. arphaman marked an inline comment as done. arphaman added a comment. Get rid of inheritance of `RefactoringEngine` by getting rid of the class itself. Repository: rL LLVM https://reviews.llvm.org/D36574 Files: include/clang/Tooling/Refactorin

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

arphaman added a comment. In https://reviews.llvm.org/D36574#858763, @klimek wrote: > One of my main concerns is still that I don't see the need for all the > template magic yet :) Why doesn't everybody use the RefactoringResult we > define here? @klimek, are you talking about the template us

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

arphaman added a comment. In https://reviews.llvm.org/D36574#860203, @klimek wrote: > In https://reviews.llvm.org/D36574#860197, @arphaman wrote: > > > In https://reviews.llvm.org/D36574#858763, @klimek wrote: > > > > > One of my main concerns is still that I don't see the need for all the > > >

[PATCH] D37390: [diagtool] Change default tree behavior to print only flags

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. LGTM. Please precommit the `auto` loop modernization though. You might want to use `llvm::make_range` for for-ranged loops as well. https://reviews.llvm.org/D37390 ___

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

arphaman updated this revision to Diff 113804. arphaman marked 8 inline comments as done. arphaman added a comment. - Test by checking the modified source directly. This allowed me to get rid of the refactoring result wrapper as well. - Remove ASTRefactoringContext - Address the other review comm

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

arphaman added inline comments. Comment at: tools/clang-refactor/ClangRefactor.cpp:60 + : SubCommand(Name, Description), Action(&Action) { +Sources = llvm::make_unique>( +cl::Positional, cl::ZeroOrMore, cl::desc(" [... ]"), arphaman wrote: > ioer

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

arphaman added inline comments. Comment at: include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h:99 + template + void invokeImplDispatch( + RefactoringResultConsumer &Consumer, RefactoringRuleContext &, ioeric wrote: > Do we still need this

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

arphaman updated this revision to Diff 113826. arphaman marked 6 inline comments as done. arphaman added a comment. - Always pass in `RefactoringRuleContext` to the refactoring rule's function. - Remove now redundant overloads. - Make LocalRename's actions members private. - Add CommandLienParser

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

arphaman added inline comments. Comment at: tools/clang-refactor/ClangRefactor.cpp:103 +IsSelectionParsed = true; +// FIXME: Support true selection ranges. +StringRef Value = *Selection; ioeric wrote: > Is the test selection temporary before we have t

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

arphaman updated this revision to Diff 113981. arphaman marked 3 inline comments as done. arphaman added a comment. Address review comments Repository: rL LLVM https://reviews.llvm.org/D36574 Files: include/clang/Tooling/Refactoring/AtomicChange.h include/clang/Tooling/Refactoring/Refact

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

arphaman marked an inline comment as done. arphaman added inline comments. Comment at: tools/clang-refactor/ClangRefactor.cpp:79 + + RefactoringAction &getAction() const { return *Action; } + ioeric wrote: > Why return a mutable reference? Also, this is not used

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

arphaman updated this revision to Diff 114125. arphaman added a comment. Refactor the selection argument handling by using a `SourceSelectionArgument` interface with subclass for test selection and separate out the test code from the tool class. Repository: rL LLVM https://reviews.llvm.org/

[PATCH] D37599: Add '\n' in ClangDataCollectorsEmitter

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. LGTM. Do you have commit access? https://reviews.llvm.org/D37599 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.or

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

arphaman updated this revision to Diff 114309. arphaman marked 2 inline comments as done. arphaman added a comment. - rename namespace - try to further decouple SourceSelectionArgument Repository: rL LLVM https://reviews.llvm.org/D36574 Files: include/clang/Tooling/Refactoring/AtomicChange

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

arphaman added inline comments. Comment at: tools/clang-refactor/ClangRefactor.cpp:62 + /// \returns true if an error occurred, false otherwise. + virtual bool refactorForEachSelection( + RefactoringRuleContext &Context, ioeric wrote: > I would expect the

[PATCH] D37618: Use CommonOptionsParser in clang-refactor

arphaman created this revision. This patch ensures that CommonOptionsParser works with subcommands. This allows clang-refactor to use the `CommonOptionsParser`. Depends on https://reviews.llvm.org/D36574. Repository: rL LLVM https://reviews.llvm.org/D37618 Files: lib/Tooling/CommonOption

<    1   2   3   4   5   6   7   8   9   10   >