[PATCH] D69263: [clangd] Implement cross-file rename.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. It's probably worth collecting a list of things we need to fix before enabling cross-file rename and putting it somewhere (a GitHub issue, maybe?) Important things that

[PATCH] D69263: [clangd] Implement cross-file rename.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:342 + // A snapshot of all file dirty buffers. + llvm::StringMap SnapShot = WorkScheduler.getAllFileContents(); auto Action = [File = File.str(), NewName = NewName.str(), Pos,

[PATCH] D69937: [clangd] Use name of Macro to compute its SymbolID.

ilya-biryukov added a comment. Sorry for not reading through and assuming the wrong thing from the title. Thanks for the explanation! LG Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69937/new/ https://reviews.llvm.org/D69937

[PATCH] D69937: [clangd] Use name of Macro to compute its SymbolID.

ilya-biryukov added a comment. Does that mean we identify each macro merely by its name? It's not uncommon to have multiple `#define` for the same name, meaning completely different things. If find references shows all of those, it's not very useful... Repository: rG LLVM Github Monorepo

[PATCH] D69934: [clangd] Implement rename by using SelectionTree and findExplicitReferences.

ilya-biryukov added a comment. It's a bit unclear how we manage to still rename overrides. Is this handled by the USR-generating functions? Could we factor out the part that collects `NamedDecl`s and use those instead of the USRs instead? Comment at:

[PATCH] D69928: [clangd] Set RetainCommentsFromSystemHeaders to true

This revision was automatically updated to reflect the committed changes. Closed by commit rG001968490049: [clangd] Set RetainCommentsFromSystemHeaders to true (authored by ilya-biryukov). Changed prior to commit: https://reviews.llvm.org/D69928?vs=228173=228186#toc Repository: rG LLVM

[PATCH] D69928: [clangd] Set RetainCommentsFromSystemHeaders to true

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM, thanks! Do you need someone to land this? Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69928/new/

[PATCH] D69896: [libTooling] Small changes in Transformer API.

ilya-biryukov accepted this revision. ilya-biryukov 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/D69896/new/ https://reviews.llvm.org/D69896

[PATCH] D69892: [clang-rename] Respect the traversal scope when traversing the entire AST.

ilya-biryukov accepted this revision. ilya-biryukov 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/D69892/new/ https://reviews.llvm.org/D69892

[PATCH] D69890: [clangd] Improve the output of rename tests where there are failures.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM! Many thanks, the new tests are much simpler to read! Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:31 + llvm::StringRef Code =

[PATCH] D69804: [clang-tidy] Update TransformerClangTidyCheck to use new Transformer bindings.

ilya-biryukov accepted this revision. ilya-biryukov 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/D69804/new/ https://reviews.llvm.org/D69804

[PATCH] D69263: [clangd] Implement cross-file rename.

ilya-biryukov added a comment. Thanks, this is in a good shape! The only concern I have is the racy way to get dirty buffers, please see the corresponding comment. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:764 + /*WantFormat=*/true, + [this](PathRef

[PATCH] D63835: [Syntax] Add nodes for most common statements

This revision was automatically updated to reflect the committed changes. Closed by commit rG58fa50f43701: [Syntax] Add nodes for most common statements (authored by ilya-biryukov). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63835/new/

[PATCH] D63835: [Syntax] Add nodes for most common statements

ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:193 + syntax::Statement *thenStatement(); + syntax::Leaf *elseKeyword(); + syntax::Statement *elseStatement(); sammccall wrote: > ilya-biryukov wrote: > > sammccall

[PATCH] D63835: [Syntax] Add nodes for most common statements

ilya-biryukov updated this revision to Diff 228023. ilya-biryukov marked 20 inline comments as done. ilya-biryukov added a comment. - Group Traverse* and Walk* together - s/RAT/RAV - Add a comment about nullability of the accessors - Name function for consuming statements and expressions

[PATCH] D69673: [clangd] Implement semantic highlightings via findExplicitReferences

This revision was automatically updated to reflect the committed changes. Closed by commit rG87e0cb4f1ad2: [clangd] Implement semantic highlightings via findExplicitReferences (authored by ilya-biryukov). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D69615: [clangd] Implement a function to lex the file to find candidate occurrences.

ilya-biryukov accepted this revision. ilya-biryukov added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/SourceCode.cpp:757 + lex(Content, LangOpts, [&](const clang::Token , const SourceManager ) { +if (Tok.getKind()

[PATCH] D69263: [clangd] Implement cross-file rename.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:107 +if (!Index) + return NoIndexProvided; + hokein wrote: > ilya-biryukov wrote: > > Why isn't this a scope enum in the first place? > this is tricky, the

[PATCH] D69608: [clangd] Helper for getting nested namespace qualification

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/AST.cpp:281 + +auto *NSD = llvm::dyn_cast(Context); +assert(NSD && "Non-namespace decl context found."); NIT: `dyn_cast` + `assert` are equivalent to a single `llvm::cast`

[PATCH] D69033: [clangd] Improve symbol qualification in DefineInline code action

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:196 //~ -> we need to qualify Bar not x. -if (!ND->getDeclContext()->isNamespace()) +if (!ND->getLexicalDeclContext()->isNamespace())

[PATCH] D69162: [clangd] Remove using-namespace present inside a namespace.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:179 + + if (ContainingNS) { +for (auto ReDeclNS : ContainingNS->redecls()) usaxena95 wrote: > ilya-biryukov wrote: > > Could you explain why

[PATCH] D69615: [clangd] Implement a function to lex the file to find candidate occurrences.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SourceCode.cpp:769 +llvm::StringRef TokenName; +if (Tok.getKind() == tok::identifier) + TokenName = Tok.getIdentifierInfo()->getName(); Why do we have both

[PATCH] D69673: [clangd] Implement semantic highlightings via findExplicitReferences

ilya-biryukov updated this revision to Diff 227713. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. - Rename to LangOpts and SourceMgr - Rename to CollectExtraHighlightings Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D69673: [clangd] Implement semantic highlightings via findExplicitReferences

ilya-biryukov marked 2 inline comments as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:220 + bool VisitOverloadExpr(OverloadExpr *E) { +if (!E->decls().empty()) + return true; // handled by

[PATCH] D69242: [Sema] Make helper in TreeTransform.h 'inline' instead of 'static'. NFC

This revision was automatically updated to reflect the committed changes. Closed by commit rG9ba16615fa07: [Sema] Make helper in TreeTransform.h inline instead of static. NFC (authored by ilya-biryukov). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D69263: [clangd] Implement cross-file rename.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:370 + for (auto : *Edits) { +if (auto Err = reformatEdit(E.getValue(), Style)) + elog("Failed to format replacements: {0}", std::move(Err)); hokein

[PATCH] D69263: [clangd] Implement cross-file rename.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:766 + [File, Code, Params, Reply = std::move(Reply), + this](llvm::Expected Edits) mutable { +if (!Edits) NIT: is capture of `this` redundant here?

[PATCH] D69263: [clangd] Implement cross-file rename.

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:274 +llvm::Expected +renameOutsideFile(const NamedDecl *RenameDecl, llvm::StringRef MainFilePath, + llvm::StringRef

[PATCH] D69673: [clangd] Implement semantic highlightings via findExplicitReferences

ilya-biryukov created this revision. ilya-biryukov added a reviewer: hokein. Herald added subscribers: usaxena95, kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. To keep the logic of finding locations of interesting AST nodes in one place. The advantage is better coverage of

[PATCH] D69632: [libTooling] Add Stencil constructor.

ilya-biryukov accepted this revision. ilya-biryukov 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/D69632/new/ https://reviews.llvm.org/D69632

[PATCH] D69624: [clangd] Fix namespace aliases in findExplicitReferences

This revision was automatically updated to reflect the committed changes. ilya-biryukov marked an inline comment as done. Closed by commit rG733777a81662: [clangd] Fix namespace aliases in findExplicitReferences (authored by ilya-biryukov). Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D69263: [clangd] Implement cross-file rename.

ilya-biryukov added a comment. Thanks, mostly LG! The only important comment I have left is about limiting the number of references. Others are NITs. Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:219 + // us whether the ref results is completed. + RQuest.Limit =

[PATCH] D69624: [clangd] Fix namespace aliases in findExplicitReferences

ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. In D69624#1728377 , @kadircet wrote: > As discussed offline, it might make more sense to fix this in `targetDecls` > itself. Considering how the `Alias` is handled for

[PATCH] D69162: [clangd] Remove using-namespace present inside a namespace.

ilya-biryukov added a comment. One important question about running on the whole TU in all cases. Other than that LG Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:110 return false; - if (!dyn_cast(TargetDirective->getDeclContext())) -

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM, many thanks! Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:293 + // Now try to generate edits for all the refs. + for (auto :

[PATCH] D69624: [clangd] Fix namespace aliases in findExplicitReferences

ilya-biryukov created this revision. ilya-biryukov added a reviewer: kadircet. Herald added subscribers: usaxena95, arphaman, jkorous, MaskRay. Herald added a project: clang. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D69624 Files: clang-tools-extra/clangd/FindTarget.cpp

[PATCH] D69162: [clangd] Remove using-namespace present inside a namespace.

ilya-biryukov added a comment. The most important comment is in the tests. Is there a way to have the same effect with less changes? Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:47 const UsingDirectiveDecl *TargetDirective = nullptr; +

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1517 +template +void foo(PARAM, TYPE b, TYPE c, TYPE d = BODY(x)){} + We fail to rename the parameter in that case, right? Should the action not be

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

ilya-biryukov added a comment. Just a few final NITs from my side. And would also be nice to get tests with macros. Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:251 +} +NewName.append(SourceParam->getName()); +

[PATCH] D69517: [clangd] Add a hidden tweak to dump symbol under the curosr.

ilya-biryukov added a comment. NIT: a typo in the title: s/curosr/cursor Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69517/new/ https://reviews.llvm.org/D69517 ___ cfe-commits mailing list

[PATCH] D69517: [clangd] Add a hidden tweak to dump symbol under the curosr.

ilya-biryukov accepted this revision. ilya-biryukov 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/D69517/new/ https://reviews.llvm.org/D69517

[PATCH] D69544: [clangd] NFC, reuse the source manager variable in the RawStringLiteral apply method

ilya-biryukov accepted this revision. ilya-biryukov 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/D69544/new/ https://reviews.llvm.org/D69544

[PATCH] D69506: [clangd] Add missing highlights for using decls.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. See the NITs, though Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:51 } + if (auto *USD = dyn_cast(D)) +return

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:233 +bool fillTemplateParameterMapping( +const FunctionDecl *Dest, const FunctionDecl *Source, +llvm::DenseMap ) { NIT: instead of returning

[PATCH] D69506: [clangd] Add missing highlights for using decls.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:51 } + if (auto *UD = dyn_cast(D)) { +if (UD->shadow_size() == 0) Could we reuse `kindForCandidateDecls` instead? It's best to keep one way to highlight

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

ilya-biryukov added a comment. D69511 broke the anonymous parameters. Sorry about that, I hope that's for the best in the long run :-) We'll need some code to update this patch. Other than that, I think this patch is good to go! Repository: rG LLVM Github

[PATCH] D69511: [clangd] Do not report anonymous entities in findExplicitReferences

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/FindTarget.cpp:453 + // and the clients are not prepared to handle that. + if (ND->getDeclName().isIdentifier() && +

[PATCH] D69511: [clangd] Do not report anonymous entities in findExplicitReferences

This revision was automatically updated to reflect the committed changes. Closed by commit rG4c430a7c6f6b: [clangd] Do not report anonymous entities in findExplicitReferences (authored by ilya-biryukov). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D69511: [clangd] Do not report anonymous entities in findExplicitReferences

ilya-biryukov added a comment. Stumbled upon it when trying to move semantic highlight to use `findExplicitReferences`. This will create some trouble for D68937 , but I believe it's actually worth it. `NameLoc` was never intended to point outside the name

[PATCH] D69511: [clangd] Do not report anonymous entities in findExplicitReferences

ilya-biryukov created this revision. ilya-biryukov added a reviewer: hokein. Herald added subscribers: usaxena95, kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. Otherwise every client dealing with name location should handle anonymous names in a special manner. This seems

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:278 +const auto *Target = +llvm::dyn_cast(Ref.Targets.front()->getCanonicalDecl()); +auto It =

[PATCH] D69431: [clangd] Do not highlight keywords in semantic highlighting

This revision was automatically updated to reflect the committed changes. Closed by commit rGc814f4c4592c: [clangd] Do not highlight keywords in semantic highlighting (authored by ilya-biryukov). Changed prior to commit: https://reviews.llvm.org/D69431?vs=226419=226632#toc Repository: rG

[PATCH] D69431: [clangd] Do not highlight keywords in semantic highlighting

ilya-biryukov added a comment. Thanks for the review! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69431/new/ https://reviews.llvm.org/D69431 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D69431: [clangd] Do not highlight keywords in semantic highlighting

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:233 - bool VisitTypeLoc(TypeLoc TL) { -if (auto K = kindForType(TL.getTypePtr())) hokein wrote: > as we are not visiting this base `TypeLoc` in this patch,

[PATCH] D69382: [clangd] Do not insert parentheses when completing a using declaration

This revision was automatically updated to reflect the committed changes. Closed by commit rGd9971d0b2e34: [clangd] Do not insert parentheses when completing a using declaration (authored by ilya-biryukov). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D69382: [clangd] Do not insert parentheses when completing a using declaration

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/CodeComplete.cpp:483 bool EnableFunctionArgSnippets; + bool CompleteArgumentList; }; kadircet wrote: > kadircet wrote: > > maybe rather use `GenerateSnippets`? Since this doesn't

[PATCH] D69382: [clangd] Do not insert parentheses when completing a using declaration

ilya-biryukov updated this revision to Diff 226620. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. - Update the comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69382/new/ https://reviews.llvm.org/D69382

[PATCH] D69263: [clangd] Implement cross-file rename.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:269 +cat(Features), +desc("Enable cross-file rename feature."), +init(false), Could you document that the feature is highly experimental and may lead to

[PATCH] D69382: [clangd] Do not insert parentheses when completing a using declaration

ilya-biryukov updated this revision to Diff 226426. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Rename flag to GenerateSnippets, document it Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69382/new/

[PATCH] D69382: [clangd] Do not insert parentheses when completing a using declaration

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1466 Output.Completions.back().Score = C.second; Output.Completions.back().CompletionTokenRange = ReplacedRange; } kadircet wrote: > why not handle

[PATCH] D69431: [clangd] Do not highlight keywords in semantic highlighting

ilya-biryukov created this revision. ilya-biryukov added a reviewer: hokein. Herald added subscribers: usaxena95, kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. Editors are good at highlightings the keywords themselves. Note that this only affects highlightings of builtin

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

ilya-biryukov added a comment. Ah, we're actually trying to rename parameters in the declaration to match the ones in the definition... So the try-catch blocks are not a problem, actually Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68937/new/

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

ilya-biryukov added a comment. In D68937#1710915 , @kadircet wrote: > I totally agree that the solution you proposed would also work, but don't > think it would be any less code. Since one needs to correlate > parameters between two different

[PATCH] D69237: Refactor getDeclAtPosition() to use SelectionTree + targetDecl()

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/XRefs.cpp:139 +// that constructor. FIXME(nridge): this should probably be handled in +// targetDecl() itself. +const CXXConstructorDecl

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

ilya-biryukov accepted this revision. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:339 + +const

[PATCH] D69382: [clangd] Do not insert parentheses when completing a using declaration

ilya-biryukov updated this revision to Diff 226293. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Use the same CodeCompletionContext Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69382/new/

[PATCH] D69382: [clangd] Do not insert parentheses when completing a using declaration

ilya-biryukov marked 3 inline comments as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:463 + // Check all-scopes completions too. + Opts.AllScopes = true; + Results = completions(R"cpp(

[PATCH] D69382: [clangd] Do not insert parentheses when completing a using declaration

ilya-biryukov updated this revision to Diff 226253. ilya-biryukov added a comment. - Fix name of a constructor parameter Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69382/new/ https://reviews.llvm.org/D69382 Files:

[PATCH] D69382: [clangd] Do not insert parentheses when completing a using declaration

ilya-biryukov created this revision. ilya-biryukov added a reviewer: kadircet. Herald added subscribers: usaxena95, arphaman, jkorous, MaskRay. Herald added a project: clang. Would be nice to also fix this in clang, but that looks like more work if we want to preserve signatures in informative

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. A few last NITs and one important comment about handling the case when function definition come from macro expansions Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:323 + +

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:208 + + if (HadErrors) { +return llvm::createStringError( kadircet wrote: > ilya-biryukov wrote: > >

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:79 + Token CurToken; + while (!CurToken.is(tok::semi)) { +if (RawLexer.LexFromRawLexer(CurToken)) kadircet wrote: > ilya-biryukov wrote: > > What are

[PATCH] D69263: [clangd] Implement cross-file rename.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:119 +llvm::Optional +renameableOutsideFile(const NamedDecl , const SymbolIndex *Index) { hokein wrote: > ilya-biryukov wrote: > > So `llvm::None` means we do not

[PATCH] D69329: [clangd] abort if shutdown takes more than a minute.

ilya-biryukov added a reviewer: ilya-biryukov. ilya-biryukov added a comment. Adding myself to the reviewers since I've already looked at it. Unless anyone objects. Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:219 + // destroyed first, to ensure worker threads

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

ilya-biryukov added a comment. Mostly comments about tests, the implementation itself LG. Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:68 +llvm::Optional getSemiColonForDecl(const FunctionDecl *FD) { + const SourceManager =

[PATCH] D69338: [clangd] Collect name references in the index.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:633 + Annotations Header(R"( +class $foo[[Foo]] { +public:

[PATCH] D69237: Refactor getDeclAtPosition() to use SelectionTree + targetDecl()

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/XRefs.cpp:139 +// that constructor. FIXME(nridge): this should probably be handled in +// targetDecl() itself. +const CXXConstructorDecl *findCalledConstructor(const SelectionTree::Node *N) {

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

ilya-biryukov updated this revision to Diff 226106. ilya-biryukov added a comment. - Add the added bit to serialization - Mention contains-errors in the AST dump. Still not tests in this revision, see D69330 for an expression that is actually preserved and has

[PATCH] D69328: [clangd] Propogate context in TUScheduler::run

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/TUScheduler.cpp:924 +Action = std::move(Action)]() mutable { Action(); }; + PreambleTasks->runAsync(Name, std::move(ActionWithCtx)); } Maybe inline

[PATCH] D69330: [AST] Add RecoveryExpr to retain expressions on semantic errors

ilya-biryukov created this revision. ilya-biryukov added a reviewer: rsmith. Herald added subscribers: usaxena95, erik.pilkington, kadircet, arphaman. Herald added a project: clang. Normally clang avoids creating expressions when it encounters semantic errors, even if the parser knows which

[PATCH] D69329: [clangd] abort if shutdown takes more than a minute.

ilya-biryukov added a comment. Not sure 60 is enough, e.g. building a preamble can often take more than a minute and the users will see clangd crashing for (seemingly) no reason on shutdown. In the long run, we should probably attempt to cancel long-running operations within a much shorter

[PATCH] D69263: [clangd] Implement cross-file rename.

ilya-biryukov added a comment. In D69263#1717985 , @hokein wrote: > Thinking more about this -- we have a dynamic index (for all opened files) > which is overlaid on a static index (which is a background index in > open-source world), so for all

[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

ilya-biryukov added a comment. Just a few NITs from my side. I'll let the other review the actual implementation Comment at: clang-tools-extra/clangd/ParsedAST.cpp:217 + void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) override { +

[PATCH] D69241: [clangd] Handle the missing constructor initializers in findExplicitReferences.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/FindTarget.cpp:677 if (const CXXCtorInitializer *CCI = N.get()) { - if (CCI->isBaseInitializer()) -return refInTypeLoc(CCI->getBaseClassLoc()); -

[PATCH] D69241: [clangd] Handle the missing constructor initializers in findExplicitReferences.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/FindTarget.cpp:677 if (const CXXCtorInitializer *CCI = N.get()) { - if (CCI->isBaseInitializer()) -return refInTypeLoc(CCI->getBaseClassLoc()); -

[PATCH] D69237: Refactor getDeclAtPosition() to use SelectionTree + targetDecl()

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/XRefs.cpp:139 +// that constructor. FIXME(nridge): this should probably be handled in +// targetDecl() itself. +const CXXConstructorDecl *findCalledConstructor(const SelectionTree::Node *N) {

[PATCH] D69241: [clangd] Handle the missing consturctor initializers in findExplicitReferences.

ilya-biryukov added a comment. NIT: there's a typo in the revision title: should be **constructor**, not **consturctor** Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69241/new/ https://reviews.llvm.org/D69241

[PATCH] D69263: [clangd] Implement cross-file rename.

ilya-biryukov added a comment. Thanks for implementing this. I think we could split it into multiple changes to make understanding it easier, see inline comments, I've tried to point out the places I find relevant. Would definitely be nice to have some unit-tests for this. Another important

[PATCH] D69165: [clangd] Store Index in Tweak::Selection

ilya-biryukov accepted this revision. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. Thanks Comment at: clang-tools-extra/clangd/refactor/Tweak.h:51 +Selection(ParsedAST , unsigned

[PATCH] D69165: [clangd] Store Index in Tweak::Selection

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:71 + // Index to be passed into Tweak::Selection. + const SymbolIndex *Index = nullptr; + kadircet wrote: > ilya-biryukov wrote: > > How is this index populated?

[PATCH] D69242: [Sema] Make helper in TreeTransform.h 'inline' instead of 'static'. NFC

ilya-biryukov created this revision. ilya-biryukov added a reviewer: Anastasia. Herald added a project: clang. ilya-biryukov retitled this revision from "[Sema] Make helper in Sema.h 'inline' instead of 'static'" to "[Sema] Make helper in TreeTransform.h 'inline' instead of 'static'. NFC".

[PATCH] D69241: [clangd] Handle the missing consturctor initializers in findExplicitReferences.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM, thanks! Comment at: clang-tools-extra/clangd/FindTarget.cpp:646 + bool TraverseConstructorInitializer(CXXCtorInitializer *Init) { +

[PATCH] D69241: [clangd] Handle the missing consturctor initializers in findExplicitReferences.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:844 +int $1^abc; +$2^X(): $3^abc() {} + }; Could you also add a test with the initializer containing a base

[PATCH] D69165: [clangd] Store Index in Tweak::Selection

ilya-biryukov added a comment. Overall LG, just a few quick questions. Comment at: clang-tools-extra/clangd/refactor/Tweak.h:65 SelectionTree ASTSelection; +/// The Index being used by ClangdServer. +const SymbolIndex *Index = nullptr; NIT: Let's

[PATCH] D69177: [clangd] Propogate FS into Tweak::Selection

ilya-biryukov added a comment. This can be obtained from the file manager inside the AST. I guess a helper function to do this would be helpful, but I don't think we need to strore another copy of it. WDYT? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D68977: [clangd] Report declaration references in findExplicitReferences.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. Many thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68977/new/ https://reviews.llvm.org/D68977

[PATCH] D68977: [clangd] Report declaration references in findExplicitReferences.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/FindTarget.cpp:495 struct Visitor : TypeLocVisitor { -llvm::Optional Ref; +llvm::SmallVector Refs; Could we keep an `llvm::Optional<>` here in the visitor? The logic in

[PATCH] D68977: [clangd] Report declaration references in findExplicitReferences.

ilya-biryukov added a comment. Mostly NITs from my side, the change LG, thanks! Comment at: clang-tools-extra/clangd/FindTarget.cpp:422 + // "using namespace" declaration doesn't have a name. + Refs.push_back({D->getQualifierLoc(), +

[PATCH] D68977: [clangd] Report declaration references in findExplicitReferences.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/FindTarget.cpp:415 Optional refInDecl(const Decl *D) { struct Visitor : ConstDeclVisitor { kadircet wrote: > ilya-biryukov wrote: > > hokein wrote: > > > ilya-biryukov wrote: > > > >

[PATCH] D68977: [clangd] Report declaration references in findExplicitReferences.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/FindTarget.cpp:438 D->getTargetNameLoc(), + /*IsDecl=*/true, {D->getAliasedNamespace()}}; This one is a **not**

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