[PATCH] D137401: [clangd] Rename QueryDriverDatabase.cpp to SystemIncludeExtractor.cpp

nridge created this revision. nridge added a reviewer: sammccall. Herald added subscribers: kadircet, arphaman. Herald added a project: All. nridge requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a project: clang-tools-extra. Repositor

[PATCH] D137401: [clangd] Rename QueryDriverDatabase.cpp to SystemIncludeExtractor.cpp

nridge updated this revision to Diff 473161. nridge added a comment. fix comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137401/new/ https://reviews.llvm.org/D137401 Files: clang-tools-extra/clangd/CMakeLists.txt clang-tools-extra/clangd

[PATCH] D137401: [clangd] Rename QueryDriverDatabase.cpp to SystemIncludeExtractor.cpp

nridge added a comment. Follow-up to D133757 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137401/new/ https://reviews.llvm.org/D137401 ___ cfe-commits mailing list cfe

[PATCH] D133757: [clangd] Perform system include extraction inside CommandMangler

nridge added a comment. In D133757#3895710 , @nridge wrote: > I will follow up with a patch to rename QueryDriverDatabase.cpp to > SystemIncludeExtractor.cpp. Posted in D137401 Repository: rG LLVM Github Monorepo

[PATCH] D131295: [clangd] Implement textDocument/codeLens

nridge added a reviewer: kadircet. nridge added a comment. Haven't had a chance to look at this yet. I do see that the earier implementation in D91930 was the subject of some design discussions about performance with @kadircet, adding him as an additional revie

[PATCH] D130015: [clangd] Add "usedAsMutablePointer" highlighting modifier

nridge added a comment. In D130015#3702004 , @ckandeler wrote: > IMO the relevant point is the (non-)const-ness of the corresponding > parameter, i.e. can the passed object get mutated or not. The fact that > there's an ampersand at the call site (which

[PATCH] D137040: [clangd] Add heuristic for dropping snippet when completing member function pointer

nridge added a comment. In D137040#3909906 , @tom-anders wrote: > We now probably also need a unit test for Sema that tests the new flag, right? Good point. My understanding is that most test coverage of libSema's code completion takes the form of lit

[PATCH] D137056: [clangd] Fix a small inconsistency in system-include-extractor.test

nridge updated this revision to Diff 473588. nridge added a comment. address nit Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137056/new/ https://reviews.llvm.org/D137056 Files: clang-tools-extra/clangd/test/system-include-extractor.test Inde

[PATCH] D137056: [clangd] Fix a small inconsistency in system-include-extractor.test

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG41fa7d2093e0: [clangd] Fix a small inconsistency in system-include-extractor.test (authored by nridge). Repository: rG LLVM Github Monorepo CHANG

[PATCH] D130015: [clangd] Add "usedAsMutablePointer" highlighting modifier

nridge accepted this revision. nridge added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:592 + Arg = IC->getSubExprAsWritten(); +if (auto *UO = dyn_cast(Arg)) { + if (UO->getOpcode()

[PATCH] D133756: [clangd] Pass the entire tooling::CompileCommand to CommandMangler

This revision was automatically updated to reflect the committed changes. Closed by commit rGafa22c563f12: [clangd] Pass the entire tooling::CompileCommand to CommandMangler (authored by nridge). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133756/n

[PATCH] D133757: [clangd] Perform system include extraction inside CommandMangler

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG68e230aa29f7: [clangd] Perform system include extraction inside CommandMangler (authored by nridge). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D137401: [clangd] Rename QueryDriverDatabase.cpp to SystemIncludeExtractor.cpp

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG428ac8f3a0f9: [clangd] Rename QueryDriverDatabase.cpp to SystemIncludeExtractor.cpp (authored by nridge). Repository: rG LLVM Github Monorepo CHA

[PATCH] D137674: [clangd] Make system-include-extractor.test more resilient in the face of paths with special characters

nridge created this revision. nridge added a reviewer: ayzhao. Herald added subscribers: kadircet, arphaman. Herald added a project: All. nridge requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a project: clang-tools-extra. Repository:

[PATCH] D133757: [clangd] Perform system include extraction inside CommandMangler

nridge added a comment. In D133757#3915995 , @ayzhao wrote: > This change is causing system-include-extractor.test > > to fail on Chrome: https:

[PATCH] D137674: [clangd] Make system-include-extractor.test more resilient in the face of paths with special characters

nridge abandoned this revision. nridge added a comment. Great minds :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137674/new/ https://reviews.llvm.org/D137674 ___ cfe-commits mailing list cfe-commits@

[PATCH] D137770: [docs] Introduce clangd part in StandardCPlusPlusModules

nridge added a comment. In D137770#3919327 , @ChuanqiXu wrote: > And we need the compiler (clang) to parse the module file from different > versions. I think it's more likely that we'll need to get clangd to build the modules itself (or by using a clan

[PATCH] D137040: [clangd] Add heuristic for dropping snippet when completing member function pointer

nridge added a comment. Thanks! In D137040#3918118 , @tom-anders wrote: > now we kinda have the same test case duplicated in sema and clangd tests - I > guess for clangd we now actually only have to test that the SnippetSuffix is > cleared when Functi

[PATCH] D137697: Move the isSelfContained function from clangd to libtooling.

nridge added a comment. nit: isSelfContained/isSelfContainedHeader in commit message Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137697/new/ https://reviews.llvm.org/D137697 ___ cfe-commits mailing lis

[PATCH] D137040: [clangd] Add heuristic for dropping snippet when completing member function pointer

nridge accepted this revision. nridge added a comment. This revision is now accepted and ready to land. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137040/new/ https://reviews.llvm.org/D137040

[PATCH] D142440: [clangd] Don't show 'auto' type hint when type deduction fails

nridge created this revision. nridge added reviewers: sammccall, kadircet. Herald added a subscriber: arphaman. Herald added a project: All. nridge requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a project: clang-tools-extra. Fixes htt

[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

nridge added a comment. In D142014#4081922 , @v1nh1shungry wrote: > I just came up with a case where the original implementation and this patch > behave differently. > > void foobar(const float &); > int main() { > foobar(0); >^ >

[PATCH] D142692: [clang] Store the template param list of an explicit variable template specialization

nridge created this revision. nridge added reviewers: rsmith, mizvekov. Herald added subscribers: kadircet, arphaman. Herald added a project: All. nridge requested review of this revision. Herald added projects: clang, clang-tools-extra. Herald added a subscriber: cfe-commits. VarTemplateSpecializ

[PATCH] D142692: [clang] Store the template param list of an explicit variable template specialization

nridge added a comment. So far the only test I added is in clangd semantic highlighting because that's how I noticed the `template<>` missing from the AST, but I'm open to suggestions on where to add a test at the clang layer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D139926: [clangd] Add semantic token for angle brackets

nridge added a comment. In D139926#4076652 , @ckandeler wrote: > Thanks for the test cases! > All fixed, except: > >> // variable template specialization >> // parameter and argument lists are missing semantic tokens >> template <> >> constexpr int V = 5;

[PATCH] D139926: [clangd] Add semantic token for angle brackets

nridge added a comment. Function calls are still missing some cases: template void free(); template struct A { template void mem(); }; void foo() { A a; a.mem(); // <-- } template void bar() { free(); // <-- A a; a.mem(); // <--

[PATCH] D139926: [clangd] Add semantic token for angle brackets

nridge added a comment. Two more cases I found: template concept C = true; template A> // <-- (inner pair) class B {}; This is a `TypeConstraint`, but RecursiveASTVisitor is lacking a Visit() method for it, so I think you'll need to override `TraverseTypeConstraint()` instead (ex

[PATCH] D139926: [clangd] Add semantic token for angle brackets

nridge added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:641 + VisitClassTemplateSpecializationDecl(ClassTemplateSpecializationDecl *D) { +for (unsigned i = 0; i < D->getNumTemplateParameterLists(); ++i) { + if (auto *TPL = D->getTempl

[PATCH] D139926: [clangd] Add semantic token for angle brackets

nridge added a comment. In D139926#4084854 , @ckandeler wrote: > I would have expected these to be handled by VisitDeclRefExpr(), but they > aren't. Any idea? There are two other expression types, `DependentScopeDeclRefExpr` and `CXXDependentScopeMembe

[PATCH] D139926: [clangd] Add semantic token for angle brackets

nridge added a comment. In D139926#4084856 , @nridge wrote: > In D139926#4084854 , @ckandeler > wrote: > >> I would have expected these to be handled by VisitDeclRefExpr(), but they >> aren't. Any idea? > > Ther

[PATCH] D139926: [clangd] Add semantic token for angle brackets

nridge added a comment. I figured I might as well look through the AST API for classes with getLAngleLoc/getRAngleLoc methods. It looks like we've got almost all of them (including the ones mentioned in recent comments) except: - OverloadExpr - DependentTemplateSpecializationTypeLoc - AutoType

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

nridge added a comment. @daiyousei-qz what is the current status of this patch? Is it ready to be merged again? (If so, I can do that for you.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127082/new/ https://reviews.llvm.org/D127082 ___

[PATCH] D142440: [clangd] Don't show 'auto' type hint when type deduction fails

nridge updated this revision to Diff 493203. nridge added a comment. address nit Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142440/new/ https://reviews.llvm.org/D142440 Files: clang-tools-extra/clangd/InlayHints.cpp clang-tools-extra/clangd

[PATCH] D142440: [clangd] Don't show 'auto' type hint when type deduction fails

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG68d7f69001c5: [clangd] Don't show 'auto' type hint when type deduction fails (authored by nridge). Repository: rG LLVM Github Monorepo CHANGES SI

[PATCH] D139926: [clangd] Add semantic token for angle brackets

nridge added a comment. In D139926#4084970 , @ckandeler wrote: > Adding AutoTypeLoc broke tons of tests, so I left it out for now. Poked at this a bit, it looks like `AutoTypeLoc.getLAngleLoc()` is only safe to access if it `isConstrained()`. This is po

[PATCH] D142871: [clangd] Semantic highlighting for constrained-parameter

nridge created this revision. nridge added a reviewer: kadircet. Herald added a subscriber: arphaman. Herald added a project: All. nridge requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a project: clang-tools-extra. Repository: rG LL

[PATCH] D142871: [clangd] Semantic highlighting for constrained-parameter

nridge added inline comments. Comment at: clang-tools-extra/clangd/FindTarget.cpp:720 + // Pick up the name via VisitNamedDecl + Base::VisitTemplateTypeParmDecl(D); +} Am I using the API correctly here? It's a bit weird that `ConstDeclVisitor` woul

[PATCH] D139926: [clangd] Add semantic token for angle brackets

nridge added inline comments. Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:1026 +concept $Concept_decl[[C2]] = true; +template $Bracket[[<]]C2$Bracket[[<]]int$Bracket[[>]] $TemplateParameter_def[[A]]$Bracket[[>]] +class $C

[PATCH] D139926: [clangd] Add semantic token for angle brackets

nridge added a comment. In D139926#4084846 , @nridge wrote: > template > class A { > template void foo(U a) { } > template<> void foo(int a) { } // <-- > }; > > This one is `ClassScopeFunctionSpecializationDecl::getTemplateArgsAsWritten

[PATCH] D139926: [clangd] Add semantic token for angle brackets

nridge added a comment. I did find one other edge case (last one, I promise!) template void foo(T); template class A { friend void foo<>(T); // <-- }; This one is `FunctionDecl::getDependentSpecializationInfo()->getLAngleLoc()` Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D139926: [clangd] Add semantic token for angle brackets

nridge added a comment. Thanks for the updates! In D139926#4064769 , @kadircet wrote: > LMK if you're going to take a look at the implementation @nridge, otherwise I > am happy to do that as well. The implementation looks good to me now. (I admit I don

[PATCH] D142692: [clang] Store the template param list of an explicit variable template specialization

nridge added a comment. In D142692#4090867 , @erichkeane wrote: > We have a good amount of AST-dump tests around that are likely the right > place to put them perhaps? What is the reproducer that caused you to > discover this issue? The following is

[PATCH] D142871: [clangd] Semantic highlighting for constrained-parameter

nridge updated this revision to Diff 494172. nridge added a comment. Address review comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142871/new/ https://reviews.llvm.org/D142871 Files: clang-tools-extra/clangd/FindTarget.cpp clang-tools-e

[PATCH] D142871: [clangd] Semantic highlighting for constrained-parameter

nridge added a comment. Also filed https://github.com/llvm/llvm-project/issues/60469 for the seemingly missing RecursiveASTVisitor methods Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142871/new/ https://reviews.llvm.org/D142871

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

nridge added a comment. In D127082#4101322 , @daiyousei-qz wrote: > In D127082#4089377 , @nridge wrote: > >> @daiyousei-qz what is the current status of this patch? Is it ready to be >> merged again? (If so, I c

[PATCH] D142077: [Clang][SemaCXX][Coroutines] Fix misleading diagnostics with -Wunsequenced

nridge added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:15186 + if (ChildExpr == CSE->getOperand()) +// Do not recurse over a CoroutineSuspendExpr's operand. +// The operand is also a subexpression of getCommonExpr(), and br

[PATCH] D142871: [clangd] Semantic highlighting for constrained-parameter

nridge updated this revision to Diff 495015. nridge added a comment. address review comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142871/new/ https://reviews.llvm.org/D142871 Files: clang-tools-extra/clangd/FindTarget.cpp clang-tools-e

[PATCH] D142871: [clangd] Semantic highlighting for constrained-parameter

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG6c4691391419: [clangd] Semantic highlighting for constrained-parameter (authored by nridge). Repository: rG LLVM Github Monorepo CHANGES SINCE LA

[PATCH] D139926: [clangd] Add semantic tokens for angle brackets

nridge added a comment. In D139926#4056478 , @kadircet wrote: > This will only make sure we have a distinction between `<<` and `>>` used as > operators vs `<`/`>` Note that `<<` and `>>` are not the only (or even the primary, in my mind) issue: there

[PATCH] D139926: [clangd] Add semantic tokens for angle brackets

nridge added a comment. Sorry, that was a bad example as it's not actually valid. Here's a slightly modified one: template class a {}; constexpr int b = 2; constexpr int c = 3; constexpr int d = 4; a e; The parser knows the first `<` opens a template-param-list because the na

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

nridge added a comment. In D138546#4053538 , @cpsauer wrote: > @nridge, I took a shot at the change you suggested. Confirming that this what > you were thinking of, removing `inferTargetAndDriverMode` from database load > and replacing it with a call to

[PATCH] D140619: ExtractFunction: support extracting expressions and selected part of it

nridge added a comment. In D140619#4058232 , @KKoovalsky wrote: > Hey! I can see that the build: > https://reviews.llvm.org/harbormaster/build/311598/ failed, but I am not sure > whether this is related to my change. Could someone take a look? I can r

[PATCH] D140619: ExtractFunction: support extracting expressions and selected part of it

nridge added a comment. By the way, I also got linker errors when trying to build this patch in a shared-libs build. Resolved by adding `clangASTMatchers` to `clang_target_link_libraries` in `clang-tools-extra/clangd/tool/CMakeLists.txt`. Repository: rG LLVM Github Monorepo CHANGES SINCE LA

[PATCH] D140619: ExtractFunction: support extracting expressions and selected part of it

nridge added a comment. In D140619#4067940 , @nridge wrote: > I can reproduce the failures locally with the patch applied. (The failures seem to have to do with `ExtractFunction` segfaulting when invoked at a particular location in the code in `check.te

[PATCH] D142077: [Clang][SemaCXX][Coroutines] Fix misleading diagnostics with -Wunsequenced

nridge added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:15186 + if (ChildExpr == CSE->getOperand()) +// Do not recurse over a CoroutineSuspendExpr's operand. +// The operand is also a subexpression of getCommonExpr(), and Ou

[PATCH] D142077: [Clang][SemaCXX][Coroutines] Fix misleading diagnostics with -Wunsequenced

nridge added inline comments. Comment at: clang/test/SemaCXX/warn-unsequenced-coro.cpp:7 + +namespace std { + Consider including `"Inputs/std-coroutine.h"` instead, as in e.g. [this test](https://searchfox.org/llvm/rev/1495210914997bcd0ca6937be0ae3cd6809b5ef5/cl

[PATCH] D141757: [clangd] allow extracting to variable for complete lambda expressions

nridge added a comment. LGTM, thanks! Comment at: clang-tools-extra/docs/ReleaseNotes.rst:81 +- The extract variable tweak gained support for extracting complete lambda expressions to a variable. + Maybe add a "Code Actions" section for this? Repository:

[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

nridge added a subscriber: adamcz. nridge added a comment. Thanks for the patch! I had a look at the code history, and it looks like the original reason for not using the param decl's type to determine the passing mode was (based on this comment )

[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

nridge added inline comments. Comment at: clang-tools-extra/clangd/Hover.cpp:1030 + } else { +if (CD->getNumParams() == 1) + PassType.PassBy = getPassMode(CD->getParamDecl(0)->getType()); This check should be `>= 1`, since the constructor co

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

nridge added a comment. In D138546#4075681 , @cpsauer wrote: > Sounds like you're not concerned, then, that the > JSONCompilationDatabasePlugin will get invoked and the results then passed to > a SystemIncludeExtractor-enabled mangler? > (I'd seen some

[PATCH] D139926: [clangd] Add semantic token for angle brackets

nridge added a comment. I played around with the patch a little, and found some cases where semantic tokens should be present but aren't: template class S { public: template class Nested; }; // explicit specialization // parameter list is missing semantic tokens template <>

[PATCH] D139926: [clangd] Add semantic token for angle brackets

nridge added a comment. One other thought that has occurred to me is that as we add more semantic tolens for punctuation, the test cases in `GetsCorrectTokens` become harder to read. What would you think about omitting punctuation tokens in the `GetsCorrectTokens` test cases (both `Operator` a

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

nridge added a comment. I will do my best to take a look this weekend. Your patience is appreciated :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124690/new/ https://reviews.llvm.org/D124690 ___ cfe-c

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

nridge added a comment. Will continue looking at the implementation tomorrow, for now a few minor comments and a suggested variation on a testcase. Comment at: clang-tools-extra/clangd/AST.h:19 #include "clang/AST/DeclObjC.h" +#include "clang/AST/Expr.h" #include "clang/AST/

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

nridge added a comment. Finished looking through the patch; I found it nicely organized and fairly easy to understand (as easy as code involving the analysis of C++ variadic templates can be, anyways :-D)! Comment at: clang-tools-extra/clangd/AST.cpp:682 + if (const auto

[PATCH] D127859: [clangd] Don't add inlay hints on std::move/forward

nridge accepted this revision. nridge added a comment. This revision is now accepted and ready to land. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127859/new/ https://reviews.llvm.org/D127859

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

nridge added a comment. Thanks for the update and the additional test cases! Comment at: clang-tools-extra/clangd/AST.cpp:682 + if (const auto *TTPD = + dyn_cast(TemplateParams.back())) { +const auto *TTPT = upsj wrote: > nridge wrote:

[PATCH] D127184: [clangd] Add to header map

nridge added a comment. Thanks for taking the time to do a systematic review! In D127184#3577165 , @falbrechtskirchinger wrote: > bits/mofunc_impl.h I see this included from `bits/move_only_function.h`, so I think `` would make sense for it. > bits/n

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

nridge added a comment. Thanks -- the patch looks quite good to me now! I will defer to @sammccall for final approval in case he has any additional feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124690/new/ https://reviews.llvm.org/D1246

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

nridge added a comment. Thank you for working on this! I've been using clangd with this patch applied for a week or so, and it seems to be working well, I haven't run into any issues. Could you please upload a patch with context, to make it easier to review? (Currently, above each hunk it says

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

nridge added a comment. In D127082#3560642 , @daiyousei-qz wrote: > Basically, anywhere the same code action isn't available. Need this be > documented somewhere? If you'd like, we could mention the ability to show macro expansions here

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

nridge added a comment. In D124690#3540704 , @upsj wrote: > Example where this pops up: > > cpp > namespace std { template T&& forward(T&); } > struct S { S(int a); }; > template > T bar(Args&&... args) { return T{std::forward(

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

nridge added a comment. Haven't had a chance to try it yet, but based on a quick glance, my suspicion is that the problem is the use of `ReferenceType::getPointeeType()`, which may do more unwrapping than we want (its implementation contains a loop

[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

nridge added a comment. In D124690#3542961 , @nridge wrote: > I would try using getPointeeTypeAsWritten() > > instead and see if that helps. I

[PATCH] D126757: Fix clang RecursiveASTVisitor for definition of XXXTemplateSpecializationDecl

nridge added a comment. Thanks for the patch! Could you add a semantic highlighting test case to SemanticHighlighting.GetsCorrectTokens as well please?

[PATCH] D126757: Fix clang RecursiveASTVisitor for definition of XXXTemplateSpecializationDecl

nridge added a comment. It seems you have uploaded just the changes relative to the previous patch. If you made a second commit, you need to `squash` it into the first and resubmit. Otherwise the test looks good, thanks! Comment at: clang-tools-extra/clangd/unittests/Semantic

[PATCH] D126757: Fix clang RecursiveASTVisitor for definition of XXXTemplateSpecializationDecl

nridge added a reviewer: sammccall. nridge added a subscriber: sammccall. nridge added a comment. Thanks, the patch looks good to me, but someone else should probably take a look and approve. Adding @sammccall who has looked at RecursiveASTVisitor recently in D120498

[PATCH] D126757: [AST] Fix clang RecursiveASTVisitor for definition of XXXTemplateSpecializationDecl

nridge added a comment. Thanks Richard for the review! @daiyousei-qz would you like me to commit the patch for you? (If so, I need a name + email for attribution, there doesn't seem to be one in the revision metadata.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:/

[PATCH] D126757: [AST] Fix clang RecursiveASTVisitor for definition of XXXTemplateSpecializationDecl

This revision was automatically updated to reflect the committed changes. Closed by commit rGc119a17e7fd6: [AST] Fix clang RecursiveASTVisitor for definition of… (authored by daiyousei-qz, committed by nridge). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D112661: [clangd] reuse preambles from other files when possible

nridge added a comment. @qchateau How should I interpret the abandoning of this patch -- did you run into a problem with the approach, or just don't have the time / interest to pursue it further? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D11266

[PATCH] D127217: [include-cleaner] Fix build error in unit test

nridge accepted this revision. nridge added a comment. This revision is now accepted and ready to land. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127217/new/ https://reviews.llvm.org/D127217

[PATCH] D127217: [include-cleaner] Fix build error in unit test

This revision was automatically updated to reflect the committed changes. Closed by commit rGd3520171844a: [include-cleaner] Fix build error in unit test (authored by ckandeler, committed by nridge). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D1272

[PATCH] D127184: [clangd] Add to header map

nridge added a comment. This change looks fine to me. I wonder though if we should be a bit more systematic about it, and try to cover other newly added libstdc++ implementation headers? I checked out gcc's git repository, and ran: $ git diff --name-only --diff-filter=A releases/gcc-9.1.0 re

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

nridge accepted this revision. nridge added a comment. This revision is now accepted and ready to land. Thank you, the updates look good! Please go ahead and merge after addressing the last minor nits. Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:594 +

[PATCH] D145843: [clangd] Add option to always insert headers with <> instead of ""

nridge added a comment. In D145843#4221826 , @nridge wrote: > Thanks for the clarification and suggested formulation. > > In D145843#4209750 , @sammccall > wrote: > >> I'd suggest something like: >> >> Style: >

[PATCH] D93829: [clangd] Support outgoing calls in call hierarchy

nridge updated this revision to Diff 557525. nridge added a comment. Rebased to apply to recent trunk Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93829/new/ https://reviews.llvm.org/D93829 Files: clang-tools-extra/clangd/ClangdLSPServer.cpp

[PATCH] D140745: Generate Config {Fragment structure, json schema, docs, YAML parser} from schema spec

nridge added a comment. In D140745#4505829 , @sammccall wrote: > ping on this one for when you have time (Just wanted to double-check, is this ping directed to me or Kadir (or both)? I haven't looked at this because Kadir has done a round of review and

[PATCH] D155381: [clangd] Allow indexing of __reserved_names outside system headers

nridge accepted this revision. nridge added a comment. This revision is now accepted and ready to land. Thanks! I agree that this no-configuration approach is nicer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155381/new/ https://reviews.llvm.org

[PATCH] D153946: [clangd] Add a flag to allow indexing of reserved identifiers

nridge added a comment. Yeah, I'm happy to go with D155381 instead. In D153946#4503585 , @sammccall wrote: > (Stupid over-flexible config system is too slow... I've learned my lesson > from that one!) The issues with

[PATCH] D154471: [clang] Add serialization support for the DynamicAllocLValue variant of APValue::LValueBase::Ptr

nridge updated this revision to Diff 542793. nridge added a comment. Add testcase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154471/new/ https://reviews.llvm.org/D154471 Files: clang/include/clang/AST/PropertiesBase.td clang/test/AST/dynami

[PATCH] D153946: [clangd] Add a flag to allow indexing of reserved identifiers

nridge abandoned this revision. nridge added a comment. Abandoning in favour of D155381 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153946/new/ https://reviews.llvm.org/D153946

[PATCH] D154471: [clang] Add serialization support for the DynamicAllocLValue variant of APValue::LValueBase::Ptr

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG2e7d711a1061: [clang] Add serialization support for the DynamicAllocLValue variant of APValue… (authored by nridge). Repository: rG LLVM Github Mo

[PATCH] D141757: [clangd] allow extracting to variable for lambda expressions

nridge added a comment. I promise I haven't forgotten about this. It's just been a challenge finding a large enough chunk of time to page in all the relevant context and think through the issues. Maybe this weekend... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://r

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

nridge added a comment. Thanks for the patch, these are nice improvements Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:258 case CodeCompletionString::CK_RightParen: + if (DropFunctionArguments && + ResultKind == CodeCompletionResult::RK_Decl

[PATCH] D141757: [clangd] allow extracting to variable for lambda expressions

nridge added a comment. Haven't looked at everything yet, but wanted to mention one thing I noticed. Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:91 + +// Local variables declared inside of the selected lambda cannot go out of +// scope. The

[PATCH] D157080: [clangd] Symbol search includes parameter types for (member) functions

nridge added a comment. I understood https://github.com/clangd/clangd/issues/1344 as being about `workspace/symbol` rather than `textDocument/documentSymbol`, though I see now that both are affected. The analysis is a bit different for the two, though: unlike `DocumentSymbol`, the result types

[PATCH] D157952: [clang] Support function pointer types with attributes when extracting parameter names for signature help

nridge created this revision. nridge added reviewers: hokein, kadircet, Qwinci. Herald added a subscriber: arphaman. Herald added a project: All. nridge requested review of this revision. Herald added subscribers: cfe-commits, wangpc, ilya-biryukov. Herald added projects: clang, clang-tools-extra.

[PATCH] D149236: [clangd] Bail gracefully if given an assembly or IR source file

nridge added a reviewer: sammccall. nridge added a comment. Adding Sam, since you're on a review roll ;) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149236/new/ https://reviews.llvm.org/D149236 ___ cfe

[PATCH] D157952: [clang] Support function pointer types with attributes when extracting parameter names for signature help

nridge updated this revision to Diff 551374. nridge added a comment. Address review comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157952/new/ https://reviews.llvm.org/D157952 Files: clang-tools-extra/clangd/unittests/CodeCompleteTests.cp

[PATCH] D157952: [clang] Support function pointer types with attributes when extracting parameter names for signature help

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGd9cb76bc4d5e: [clang] Support function pointer types with attributes when extracting… (authored by nridge). Repository: rG LLVM Github Monorepo C

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