[PATCH] D36083: [utils] Add a script that runs clang in LLDB and stops it when a specified diagnostic is emitted

arphaman added a comment. It might make sense to add a breakpoint at `PartialDiagnostic(unsigned DiagID, StorageAllocator &Allocator)`, I'll check how that works. I reckon it should be possible to have a script that could find the name of all emitted diagnostics. Let's say we'd like to run clan

[PATCH] D35012: [refactor] Add the AST source selection component

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

[PATCH] D36150: [clangd] LSP extension to switch between source/header file

arphaman added inline comments. Comment at: clangd/ClangdServer.cpp:292 + + if (path.compare(path.length() - 4, 4, ".cpp") == 0) { +path = path.substr(0, (path.length() - 4)); malaperle wrote: > this won't work for other extensions, we need to make this more

[PATCH] D36156: [rename] Introduce symbol occurrences

arphaman created this revision. Herald added a subscriber: mgorny. Symbol occurrences store the results of local rename and will also be used for the global, indexed rename results. They can be converted to a set of `AtomicChanges` as well. This is a preparation patch for both the support of mu

[PATCH] D35925: [Coverage] Precise region termination with deferred regions

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. Thanks, it does make sense to update llvm-cov. LGTM: Comment at: lib/CodeGen/CoverageMappingGen.cpp:554 + // useful to try and create a deferred region inside o

[PATCH] D36083: [utils] Add a script that runs clang in LLDB and stops it when a specified diagnostic is emitted

arphaman added a comment. Makes sense. I'll see if I can get somewhere with the regex idea. Repository: rL LLVM https://reviews.llvm.org/D36083 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo

[PATCH] D36156: [rename] Introduce symbol occurrences

arphaman added a comment. In https://reviews.llvm.org/D36156#827733, @kimgr wrote: > High-level comment from the peanut gallery: > > The word "occurrences" is horrible to type and most people misspell it (you > did great here!) Would you consider something like "SymbolMatches" or even > "Symbol

[PATCH] D36191: [CodeGen] Don't make availability attributes imply default visibility on macos

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. LGTM. Please document this change in the release notes. https://reviews.llvm.org/D36191 ___ cfe-commits mailing list cfe-commits@lists.llvm.o

[PATCH] D36112: [ubsan] Have -fsanitize=vptr emit a null check if -fsanitize=null isn't available

arphaman added a comment. That makes sense. It's kinda weird not to report the `null`, but I guess it makes sense if the `null` sanitiser is off. It's not actually UB unless it's dereferenced, right, so casts are allowed? Comment at: test/CodeGenCXX/ubsan-type-checks.cpp:73 +

[PATCH] D36156: [rename] Introduce symbol occurrences

arphaman added inline comments. Comment at: include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h:65-68 + ArrayRef getNameLocations() const { return Locations; } + ArrayRef getNameLengths() const { +return llvm::makeArrayRef(NameLengths, Locations.size()); + } -

[PATCH] D36176: [clang-diff] Fix some errors and inconsistencies

arphaman added inline comments. Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:96 : TreeImpl(llvm::make_unique(this, Node, AST)) {} + SyntaxTree(const SyntaxTree &Tree) = delete; ~SyntaxTree(); johannes wrote: > arphaman wrote: > > It might be bet

[PATCH] D36156: [rename] Introduce symbol occurrences

arphaman added inline comments. Comment at: include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h:65-68 + ArrayRef getNameLocations() const { return Locations; } + ArrayRef getNameLengths() const { +return llvm::makeArrayRef(NameLengths, Locations.size()); + } -

[PATCH] D36156: [rename] Introduce symbol occurrences

arphaman updated this revision to Diff 109326. arphaman added a comment. Address review comments. Repository: rL LLVM https://reviews.llvm.org/D36156 Files: include/clang/Tooling/Refactoring/Rename/RenamingAction.h include/clang/Tooling/Refactoring/Rename/SymbolName.h include/clang/Too

[PATCH] D36200: [Sema] Improve some -Wunguarded-availability diagnostic messages

arphaman added a comment. This needs a test for the fixits as well, see test/FixIt/fixit-availability* https://reviews.llvm.org/D36200 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36200: [Sema] Improve some -Wunguarded-availability diagnostic messages

arphaman added a comment. Right, sorry, I didn't realize that code was moved. https://reviews.llvm.org/D36200 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36252: [diagtool] Add ability to get name from id

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

[PATCH] D27827: [ObjC] CodeGen support for @available on macOS

arphaman added a comment. In https://reviews.llvm.org/D27827#829661, @thakis wrote: > We just noticed that if you call __builtin_available() for the first time > after activating your app's sandbox, the function will fail: > > SandboxViolation: crdmg(15489) deny file-read-data > /System/Library

[PATCH] D36177: [clang-diff] Add commandline arguments.

arphaman added inline comments. Comment at: test/Tooling/clang-diff-args.sh:1 +RUN: echo a > %t.cpp + I think a '.test' extension rather than '.sh' is better Comment at: test/Tooling/clang-diff-args.sh:3 + +RUN: echo "CHECK: unknown type name '

[PATCH] D36178: [clang-diff] Move the JSON export function to clang-diff

arphaman added inline comments. Comment at: test/Tooling/clang-diff-json.cpp:1 +// RUN: clang-diff -ast-dump %s -- | python -m json.tool | FileCheck %s + I think you have to use `%python` instead of `python`, like LLVM tests do. https://reviews.llvm.org/D36178

[PATCH] D36181: [clang-diff] Make printing of matches optional

arphaman added inline comments. Comment at: test/Tooling/clang-diff-args.sh:10 + +RUN: echo "// CHECK-NOT: {{.}}" > %t-no-output +RUN: clang-diff %S/clang-diff-ast.cpp %S/clang-diff-ast.cpp -- 2>&1 -std=c++11 \ You can use `MYCHECK-NOT` in this file and run `Fil

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

arphaman added inline comments. Comment at: clangd/ClangdServer.h:107 +public: + /// Indicates that requests must be executed immidieately on the calling + /// thread. Typo: immediately https://reviews.llvm.org/D36261 __

[PATCH] D36178: [clang-diff] Move the JSON export function to clang-diff

arphaman added inline comments. Comment at: test/Tooling/clang-diff-json.cpp:1 +// RUN: clang-diff -ast-dump %s -- | python -m json.tool | FileCheck %s + johannes wrote: > arphaman wrote: > > I think you have to use `%python` instead of `python`, like LLVM tests

[PATCH] D36177: [clang-diff] Add commandline arguments.

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. I think you have to test the -p option too (check the compilation database can be loaded and arguments are propagated to the parser), otherwise LGTM! https://reviews.llvm.org/D36177 __

[PATCH] D36181: [clang-diff] Make printing of matches optional

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

[PATCH] D36178: [clang-diff] Move the JSON export function to clang-diff

arphaman added inline comments. Comment at: test/Tooling/clang-diff-json.cpp:1 +// RUN: clang-diff -ast-dump %s -- | python -m json.tool | FileCheck %s + arphaman wrote: > johannes wrote: > > johannes wrote: > > > arphaman wrote: > > > > I think you have to use `

[PATCH] D36178: [clang-diff] Move the JSON export function to clang-diff

arphaman added a comment. @johannes , you have to add %python substitution to lit.cfg, see my attached patch that does it.F4172676: python.diff https://reviews.llvm.org/D36178 ___ cfe-commits mailing list cfe-com

[PATCH] D36176: [clang-diff] Fix some errors and inconsistencies

arphaman added a comment. LGTM. Although I'm not 100% sure it's fully NFC, so if you can't come up with a test please justify why. https://reviews.llvm.org/D36176 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin

[PATCH] D36176: [clang-diff] Fix some errors and inconsistencies

arphaman added a comment. Ok, please include this info in the commit message. https://reviews.llvm.org/D36176 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36178: [clang-diff] Move the JSON export function to clang-diff

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

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

arphaman added a comment. Don't you need `// UNSUPPORTED: c++98, c++03` since `std::function` is supported in C++11 only? https://reviews.llvm.org/D34331 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

arphaman added a comment. Hi @gtbercea, I couldn't reply to the email as cfe-commits didn't even register this commit somehow, so I'm replying here. Unfortunately I had to revert this commit (r310291), + two others for a clean revert (r310300 and r310332) because it caused a test failure on mac

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

arphaman added a comment. The revert commit is r310345 btw https://reviews.llvm.org/D29654 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

arphaman added a comment. Great, thanks! I think that you can just revert my revert with the fix applied in one commit https://reviews.llvm.org/D29654 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/lis

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

arphaman added a comment. Looks like it's still failing unfortunately: http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental_check/39182/console https://reviews.llvm.org/D29654 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

arphaman added a comment. The last RUN line in the new commit triggers the same assertion failure: Assertion failed: (CachedResults.find(ActionTC) != CachedResults.end() && "Result does not exist??"), function BuildJobsForActionNoCache, file /Users/alex/bisect/llvm/tools/clang/lib/Driver/Driv

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

arphaman added a comment. In https://reviews.llvm.org/D29654#835392, @gtbercea wrote: > In https://reviews.llvm.org/D29654#835371, @arphaman wrote: > > > The last RUN line in the new commit triggers the same assertion failure: > > > > Assertion failed: (CachedResults.find(ActionTC) != CachedRes

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

arphaman added a comment. The cached results map doesn't have the key: (lldb) p CachedResults (std::__1::map, std::__1::allocator > >, clang::driver::InputInfo, std::__1::less, std::__1::allocator > > >, std::__1::allocator, std::__1::allocator > >, clang::driver::InputInfo> > >) $0 = siz

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

arphaman added a comment. If you look at the map then you can see that it contains very similar keys, but not the exact one: first = { first = 0x000111c017d0 second = "x86_64-apple-darwin17.0.0-host" } and first = { first = 0x000111c01320

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

arphaman added a comment. The "x86_64-apple-darwin17.0.0-x86_64-host" triple looks suspicious though https://reviews.llvm.org/D29654 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

arphaman added a comment. In https://reviews.llvm.org/D29654#835501, @gtbercea wrote: > Is that the last access to CachedResults before the error? Is the assertion the last access? Yes. There must be a discrepancy between `UI.DependentBoundArch` in the loop above and `BoundArch` that's used t

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

arphaman added a comment. No, entry 1 has a different pointer (0x000111c01320 vs 0x000111c017d0). Why is there a `DependentBoundArch` if it's always empty? https://reviews.llvm.org/D29654 ___ cfe-commits mailing list cfe-commits@lists.llvm.

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

arphaman added a comment. The tests actually do compile in C++03, but they still fail because of infinite recursion: frame #196562: 0x0001155c nullptr_t_assign_reentrant.pass.cpp.exe`A::~A() + 92 frame #196563: 0x00011405 nullptr_t_assign_reentrant.pass.cpp.exe`std::__1::__

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

arphaman added a comment. If you look at the AST diff between C++11 and C++03 this error still happens because of how `nullptr` is processed: C++11: | |-CXXDestructorDecl 0x7fdab27a2498 line:24:3 used ~A 'void (void) noexcept' | | `-CompoundStmt 0x7fdab27bcfb8 | | |-GCCAsmStmt 0x7fda

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

arphaman added a comment. Ah, I got it. There's a `__functional_03` header that seems to implement `function` for C++03 because of manual variadic template expansions. You have to update the operators in the header as well. https://reviews.llvm.org/D34331

[PATCH] D36179: [clang-diff] Move printing of matches and changes to clang-diff

arphaman added a comment. Previous `Move` resulted in `llvm_unreachable("TODO");`, but now it seems to be fixed. The new output for `Move` and `UpdateMove` should be tested in a test. https://reviews.llvm.org/D36179 ___ cfe-commits mailing list cfe

[PATCH] D36180: [clang-diff] Add option to dump the AST, one node per line

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

[PATCH] D36182: [clang-diff] Add HTML side-by-side diff output

arphaman added inline comments. Comment at: test/Tooling/clang-diff-html.py:1 +# RUN: clang-diff %S/Inputs/clang-diff-basic-src.cpp %S/clang-diff-basic.cpp -html -- | %python %s > %t.filecheck +# RUN: clang-diff %S/Inputs/clang-diff-basic-src.cpp %S/clang-diff-basic.cpp -dump-m

[PATCH] D36183: [clang-diff] Simplify mapping

arphaman added inline comments. Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:114 - /// If this is set to true, nodes that have parents that must not be matched - /// (see NodeComparison) will be allowed to be matched. - bool EnableMatchingWithUnmatchableParents = fals

[PATCH] D36184: [clang-diff] Filter AST nodes

arphaman added inline comments. Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:177 +static bool isDeclExcluded(const Decl *D) { return D->isImplicit(); } +static bool isStmtExcluded(const Stmt *S) { return false; } + You should just use one call `isNodeExcluded` th

[PATCH] D29660: [OpenMP] Add flag for overwriting default PTX version for OpenMP targets

arphaman added a comment. Looks like this test is failing on macOS again after this change: http://lab.llvm.org:8080/green/job/clang-stage1-cmake-RA-incremental_check/39231/testReport/Clang/Driver/openmp_offload_c/ Can you please take a look? Repository: rL LLVM https://reviews.llvm.org/D29

[PATCH] D29660: [OpenMP] Add flag for overwriting default PTX version for OpenMP targets

arphaman added a comment. 1. I'm sorry, but I had to revert r310489 and follow-up commits r310505, r310519, r310537 and r310549 since it looks like the failures are accumulating. The revert commit was r310580. The following run lines were failing for me because of various assertion failures and

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

arphaman added a comment. + Introduce refactoring diagnostics. Repository: rL LLVM https://reviews.llvm.org/D36075 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

arphaman updated this revision to Diff 110563. arphaman edited the summary of this revision. arphaman added a comment. - Simplify error/diagnostic handling. Use `DiagnosticOr` instead of `DiagOr`. - Simplify the code for the selection requirements by removing lambda deducers and instead using sp

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

arphaman created this revision. Herald added a subscriber: mgorny. This patch depends on https://reviews.llvm.org/D36075 and https://reviews.llvm.org/D36156. It introduces the `clang-refactor` tool alongside the `local-rename` action which is uses the existing renaming engine used by `clang-ren

[PATCH] D36530: [Parse] Document PrintStats, SkipFunctionBodies

arphaman added a comment. Generally such NFC patches can be committed directly as they can be reviewed after the commit Repository: rL LLVM https://reviews.llvm.org/D36530 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm

[PATCH] D29660: [OpenMP] Add flag for overwriting default PTX version for OpenMP targets

arphaman added a comment. The failures were very widespread, e.g. there's a linux buildbot that was red until the revert: http://bb.pgr.jp/builders/test-clang-i686-linux-RA. If you have access to a linux machine you should be able to reproduce the failures that the bot experienced by using the

[PATCH] D29660: [OpenMP] Add flag for overwriting default PTX version for OpenMP targets

arphaman added a comment. I've traced the output across all the reverted commits: F5233517: testFailures.md Note that after r310549 the last 9 RUN lines started failing because of the same crash: clang version 6.0.0 (http://llvm.org/git/llvm.git 00708415

[PATCH] D36179: [clang-diff] Move printing of matches and changes to clang-diff

arphaman added inline comments. Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:40 NodeId Parent, LeftMostDescendant, RightMostDescendant; - int Depth, Height; + int Depth, Height, Shift; ast_type_traits::DynTypedNode ASTNode; Looks like `Shift` isn't

[PATCH] D36183: [clang-diff] Simplify mapping

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

[PATCH] D36184: [clang-diff] Filter AST nodes

arphaman added inline comments. Comment at: test/Tooling/clang-diff-ast.cpp:68 +// nodes from other files are excluded +// CHECK-NOT {{.}} +#include "clang-diff-ast.cpp" 1) Missing ':' 2) What exactly does this regex accomplish? Right now it will match any chara

[PATCH] D36185: [clang-diff] Fix similarity computation

arphaman added inline comments. Comment at: tools/clang-diff/ClangDiff.cpp:436 +else if (StopAfter != "bottomup") { + llvm::errs() << "Error: Invalid argument for -stop-after"; + return 1; Add a newline to the string as well. https://reviews.llvm.

[PATCH] D36610: [Tooling] Add option to getFullyQualifiedName using a custom PritingPolicy

arphaman added inline comments. Comment at: unittests/Tooling/QualTypeNamesTest.cpp:247 + "struct (anonymous struct at input.cc:1:1)"; + PrintingPolicy.runOver( + "struct\n" You should probably use multiline string here, i.e. R(" ... ") https://revie

[PATCH] D36184: [clang-diff] Filter AST nodes

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. LGTM. Comment at: test/Tooling/clang-diff-ast.cpp:68 +// nodes from other files are excluded +// CHECK-NOT {{.}} +#include "clang-diff-ast.cpp" johannes

[PATCH] D35948: [CommonOptionsParser] Expose ArgumentsAdjustingCompilationDatabase

arphaman added a reviewer: klimek. arphaman added a comment. @klimek Ping. https://reviews.llvm.org/D35948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36186: [clang-diff] Improve and test getNodeValue

arphaman added inline comments. Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:428 +Value += getRelativeName(V) + "(" + V->getType().getAsString(TypePP) + ")"; +if (auto *C = dyn_cast(D)) { + for (auto *Init : C->inits()) { It looks like you're doing a

[PATCH] D36156: [rename] Introduce symbol occurrences

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

[PATCH] D36179: [clang-diff] Move printing of matches and changes to clang-diff

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. LGTM, with one request below: Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:96 - /// Finds an edit script that converts T1 to T2. - std::vector computeChanges(Mapping &

[PATCH] D36182: [clang-diff] Add HTML side-by-side diff output

arphaman added a comment. Awesome, thanks! LGTM. https://reviews.llvm.org/D36182 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36185: [clang-diff] Fix similarity computation

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. LGTM. Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:738 + } + double Denominator = T1.getNumberOfDescendants(Id1) - 1 + + T2.getNumberOfDescendants(

[PATCH] D36156: [rename] Introduce symbol occurrences

arphaman marked 5 inline comments as done. arphaman added inline comments. Comment at: include/clang/Tooling/Refactoring/Rename/SymbolName.h:19 +namespace clang { +namespace tooling { + hokein wrote: > An off-topic thought: currently we put everything into `clang

[PATCH] D36156: [rename] Introduce symbol occurrences

arphaman updated this revision to Diff 110954. arphaman marked an inline comment as done. arphaman added a comment. Address review comments. Repository: rL LLVM https://reviews.llvm.org/D36156 Files: include/clang/Tooling/Refactoring/Rename/RenamingAction.h include/clang/Tooling/Refactor

[PATCH] D36186: [clang-diff] Improve and test getNodeValue

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. Perfect, thanks! https://reviews.llvm.org/D36186 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailma

[PATCH] D36187: [clang-diff] Use the relative name for NamedDecls

arphaman added a comment. Please rebase, it doesn't apply cleanly anymore. https://reviews.llvm.org/D36187 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36156: [rename] Introduce symbol occurrences

arphaman marked an inline comment as done. arphaman added inline comments. Comment at: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp:398 Visitor.TraverseDecl(Decl); - return Visitor.getLocationsFound(); + return std::move(Visitor.getOccurrences()); } hoke

[PATCH] D36156: [rename] Introduce symbol occurrences

arphaman updated this revision to Diff 110964. arphaman marked an inline comment as done. arphaman added a comment. Use `takeOccurrences` Repository: rL LLVM https://reviews.llvm.org/D36156 Files: include/clang/Tooling/Refactoring/Rename/RenamingAction.h include/clang/Tooling/Refactoring

[PATCH] D36686: [clang-diff] Add option to compare files across git revisions

arphaman added a comment. It might be useful to have both source and destination in a different revision. Maybe something like `-src-git-rev` and `-dst-git-rev`? https://reviews.llvm.org/D36686 ___ cfe-commits mailing list cfe-commits@lists.llvm.or

[PATCH] D36156: [rename] Introduce symbol occurrences

This revision was automatically updated to reflect the committed changes. Closed by commit rL310853: [rename] Introduce symbol occurrences (authored by arphaman). Changed prior to commit: https://reviews.llvm.org/D36156?vs=110964&id=110999#toc Repository: rL LLVM https://reviews.llvm.org/D3

[PATCH] D36200: [Sema] Improve some -Wunguarded-availability diagnostic messages

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. Sorry for the delay, LGTM https://reviews.llvm.org/D36200 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-b

[PATCH] D36728: Switch to consumeError(), since this can crash otherwise.

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

[PATCH] D36686: [clang-diff] Add option to compare files across git revisions

arphaman added a comment. That would be nice. Temporary checkouts are a way to solve the problem. But they might not work for all projects, like when some translation unit includes a file that's not in git using a relative path. Another solution is to use virtual file systems. A custom vfs coul

[PATCH] D36664: [analyzer] Make StmtDataCollector customizable

arphaman added a subscriber: rsmith. arphaman added a comment. In https://reviews.llvm.org/D36664#841758, @teemperor wrote: > Very well done, I really like this patch! I added a few remarks mostly about > the comments that need some small adjusting. > > I'm wondering what would be a nice way of

[PATCH] D36715: [clang] minor cleanup in clang/tooling/refactoring

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. LGTM Repository: rL LLVM https://reviews.llvm.org/D36715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi

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

arphaman created this revision. Prior to this patch, messages to `self` in class methods were treated as instance methods to a `Class` value. When these methods returned `instancetype` the compiler only saw `id` through the `instancetype`, and not the `Interface *`. This caused problems when th

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

arphaman added a comment. Of course, right. I will change the approach. 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] D36790: [ObjC] Messages to 'self' in class methods should use class method dispatch to avoid multiple method ambiguities

arphaman added a comment. @rjmccall Do you think that the rules for the return types in overridden methods that return `instancetype` should be strengthened first? For example, if we have the following code: @interface Unrelated - (void)method:(int)x; @end @interface CallsSelfSuper:

[PATCH] D36777: [Sema] Don't emit -Wunguarded-availability for switch cases

arphaman added a comment. Thanks! There are two issues that I see: Comment at: lib/Sema/SemaDeclAttr.cpp:7519 +// to any useful diagnostics. +for (Stmt *Child : llvm::drop_begin(CS->children(), 1)) + if (!Base::TraverseStmt(Child)) GNU case stateme

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

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

[PATCH] D36777: [Sema] Don't emit -Wunguarded-availability for switch cases

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

[PATCH] D34992: Emit static constexpr member as available_externally definition

arphaman added a comment. FYI, this doesn't compiler after John's constant emitter refactoring (r310964) https://reviews.llvm.org/D34992 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi

[PATCH] D36876: [IRGen] Evaluate constant static variables referenced through member expressions

arphaman created this revision. C++ allows us to reference static variables through member expressions. Prior to this patch, non-integer static variables that were referenced using a member expression were always emitted using lvalue loads. The old behaviour introduced an inconsistency between

[PATCH] D36187: [clang-diff] Use the relative name for NamedDecls

arphaman added inline comments. Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:371 + std::string ContextPrefix; + if (auto *Namespace = dyn_cast(Context)) +ContextPrefix = Namespace->getQualifiedNameAsString(); You don't need to check both `NamespaceDecl` and

[PATCH] D36853: [Parser] Correct initalizer typos before lambda capture type is deduced.

arphaman added a comment. There's also a crash with the `( ... )` initializer: namespace init_capture_undeclared_identifier { auto a = [x = y]{}; int typo_foo; auto b = [x (typo_boo )]{}; } I think you should move the added correction code before `if (Init.isUsable())` to en

[PATCH] D36150: [clangd] LSP extension to switch between source/header file

arphaman added inline comments. Comment at: clangd/ClangdServer.cpp:296 + + const int sourceSize = sizeof(DEFAULT_SOURCE_EXTENSIONS) / sizeof(DEFAULT_SOURCE_EXTENSIONS[0]); + const int headerSize = sizeof(DEFAULT_HEADER_EXTENSIONS) / sizeof(DEFAULT_HEADER_EXTENSIONS[0]);

[PATCH] D36853: [Parser] Correct initalizer typos before lambda capture type is deduced.

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. LGTM, Thanks. You should obtain commit access if you haven't already done so (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access). I can commit this on your behalf as well

[PATCH] D36876: [IRGen] Evaluate constant static variables referenced through member expressions

arphaman added inline comments. Comment at: lib/CodeGen/CGExprComplex.cpp:156 +auto Constant = tryEmitDeclRefOrMemberExprAsConstant( +ME, ME->getMemberDecl(), /*AllowSideEffects=*/true); +if (Constant) { rsmith wrote: > If we can (correctly) allow

[PATCH] D36876: [IRGen] Evaluate constant static variables referenced through member expressions

arphaman updated this revision to Diff 111956. arphaman marked an inline comment as done. arphaman added a comment. - Get rid of the `AllowSideEffects` argument. - Get rid of the old integer field evaluation code. Repository: rL LLVM https://reviews.llvm.org/D36876 Files: lib/CodeGen/CGExp

[PATCH] D36969: [Basic] Add a DiagnosticOr type

arphaman created this revision. Clang's `DiagnosticOr` type mimics the `Expected` type from LLVM. It either stores a partial diagnostic and its location or a value. I'll be using in https://reviews.llvm.org/D36075. Repository: rL LLVM https://reviews.llvm.org/D36969 Files: include/clang/

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

arphaman added a comment. Extracted `DiagnosticOr` to a separate patch at https://reviews.llvm.org/D36969. I will update this patch tomorrow. Repository: rL LLVM https://reviews.llvm.org/D36075 ___ cfe-commits mailing list cfe-commits@lists.llvm

[PATCH] D36187: [clang-diff] Use the relative name for NamedDecls

arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. LG Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:371 + std::string ContextPrefix; + if (auto *Namespace = dyn_cast(Context)) +ContextPrefix = Namespace->getQualifiedN

[PATCH] D36876: [IRGen] Evaluate constant static variables referenced through member expressions

arphaman added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:1323 + if (result.HasSideEffects && !AllowSideEffects) { +assert(!isa(E) && "declrefs should not have side effects"); return ConstantEmission(); rjmccall wrote: > The side effects here a

<    3   4   5   6   7   8   9   10   11   12   >