[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

ilya-biryukov added a comment. I'm somewhat familiar with the internals of clang around the FileManager/VFS, so I could try creating the repro of this issue. The bugreport has enough info to get started. (Probably tomorrow, I certainly won't get to it today). In https://reviews.llvm.org/D48687#

[PATCH] D48880: [Sema] Fix crash in getConstructorName.

ilya-biryukov created this revision. ilya-biryukov added reviewers: bkramer, rsmith. Can happen when getConstructorName is called on invalid decls, specifically the ones that do not have the injected class name. Repository: rC Clang https://reviews.llvm.org/D48880 Files: lib/Sema/SemaExprC

[PATCH] D48880: [Sema] Fix crash in getConstructorName.

ilya-biryukov updated this revision to Diff 154063. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Better recovery for invalid decls that do have an injected class name - Move the test to SemaCXX Repository: rC Clang https://reviews.llvm.org/D48880 Files:

[PATCH] D48880: [Sema] Fix crash in getConstructorName.

ilya-biryukov added a comment. Thanks for taking a look! Repository: rC Clang https://reviews.llvm.org/D48880 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48917: [SemaCodeComplete] Make sure visited contexts are passed to completion results handler.

ilya-biryukov added a comment. Generally LG, just one comment. Comment at: lib/Sema/SemaCodeComplete.cpp:3744 AddMacroResults(PP, Results, false, PreferredTypeIsPointer); - HandleCodeCompleteResults(this, CodeCompleter, -CodeCompletionContext(CodeCompleti

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

ilya-biryukov added a reviewer: ilya-biryukov. ilya-biryukov added a comment. Mimicing RealFS seems like the right thing to do here, so I would vouch for checking this change in. I'm a little worried that there are tests/clients relying on the old behavior, have you run all the tests? Also, cou

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

ilya-biryukov added a comment. > I think the fixing way is to normalize the file path from AST (making it > absolute). Totally agree. Could we run the code used to get the URI to store in the dynamic index? Should we expose and reuse code in `getSymbolLocation()` from `SymbolCollector.cpp`?

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

ilya-biryukov added a comment. Sorry, have missed the @hokein 's comments, so one of mine seems like a duplicate. Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/

[PATCH] D48881: [clangd] Avoid collecting symbols from broken TUs in global-symbol-builder.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM with a minor NIT Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:88 +(CI.getDiagnosticClient().getNumErrors() > 0)) { +

[PATCH] D48917: [SemaCodeComplete] Make sure visited contexts are passed to completion results handler.

ilya-biryukov added inline comments. Comment at: lib/Sema/SemaCodeComplete.cpp:3744 AddMacroResults(PP, Results, false, PreferredTypeIsPointer); - HandleCodeCompleteResults(this, CodeCompleter, -CodeCompletionContext(CodeCompletionContext::CCC_Expression,

[PATCH] D48917: [SemaCodeComplete] Make sure visited contexts are passed to completion results handler.

ilya-biryukov added inline comments. Comment at: lib/Sema/SemaCodeComplete.cpp:3744 AddMacroResults(PP, Results, false, PreferredTypeIsPointer); - HandleCodeCompleteResults(this, CodeCompleter, -CodeCompletionContext(CodeCompletionContext::CCC_Expression,

[PATCH] D48881: [clangd] Avoid collecting symbols from broken TUs in global-symbol-builder.

ilya-biryukov added inline comments. Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:88 +(CI.getDiagnosticClient().getNumErrors() > 0)) { + llvm::errs() << "Found errors in the translation unit. Igoring " + "colle

[PATCH] D48917: [SemaCodeComplete] Make sure visited contexts are passed to completion results handler.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang https://reviews.llvm.org/D48917 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.l

[PATCH] D48314: [Frontend] Cache preamble-related data

ilya-biryukov added a comment. I would argue this should be handled by the clients instead. Adding global state and locking is complicated. (And ASTUnit is complicated enough). What are the use-cases of creating multiple `ASTUnit` inside the same process for the same file? Which clients do that

[PATCH] D48314: [Frontend] Cache preamble-related data

ilya-biryukov added a comment. I agree that the optimization is compelling, we do also share preamble in clangd and run code completion in parallel with other actions that need an AST. However, I would argue that a proper way to introduce the optimization would be to change interface of `ASTUnit

[PATCH] D48940: [clangd] Wait for first preamble before code completion

ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added subscribers: jkorous, MaskRay, ioeric, javed.absar. To avoid doing extra work of processing headers in the preamble mutilple times in parallel. Repository: rCTE Clang Tools Extra https://reviews.llvm.

[PATCH] D48942: [PCH] Add an option to not write comments into PCH

ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added a subscriber: ioeric. Will be used in clangd, see the follow-up change. Clangd does not use comments read from PCH to avoid crashes due to changed contents of the file. However, reading them considerably s

[PATCH] D48943: [clangd] Do not write comments into Preamble PCH

ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added subscribers: jkorous, MaskRay, ioeric. To avoid wasting time deserializing them on code completion and further reparses. We do not use the comments anyway, because we cannot rely on the file contents stay

[PATCH] D48942: [PCH] Add an option to not write comments into PCH

ilya-biryukov added a comment. Not sure about two things: - Using PreprocessorOptions for plumbing this setting. I'd rather put it into FrontendOptions, but there seems to be no way to get those in the ASTWriter... Happy to search for the alternatives - Lack of tests. I'm not sure how to proper

[PATCH] D48946: [Preamble] Check system dependencies in preamble too

ilya-biryukov created this revision. ilya-biryukov added reviewers: sammccall, ioeric. PrecompiledPreamble hasn't checked the system dependencies changed before. This result in invalid preamble not being rebuilt if headers that changed were found in -isystem include paths. This pattern is sometim

[PATCH] D48947: [clangd] Added a test for preambles and -isystem

ilya-biryukov created this revision. ilya-biryukov added reviewers: sammccall, ioeric. Herald added subscribers: jkorous, MaskRay. Checks that preambles are properly invalidated when headers from -isystem paths change. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48947 Files:

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

ilya-biryukov added a comment. I usually run `ninja check-clang check-clang-tools` for clang changes. Have never used `clang-test`, not sure what it does. I ran it with this change, found a few failures from clang-move: Failing Tests (5): Extra Tools Unit Tests :: clang-move/./ClangMove

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

ilya-biryukov added a comment. In https://reviews.llvm.org/D48903#1154846, @simark wrote: > With the `InMemoryFileSystem`, this now returns a non-real path. The result > is that we fill `RealPathName` with that non-real path. I see two options > here: > > 1. Either the FileManager is wrong to

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

ilya-biryukov added inline comments. Comment at: lib/Basic/VirtualFileSystem.cpp:488 + } + StringRef getName() const { return Stat.getName(); } InMemoryNodeKind getKind() const { return Kind; } Given that this method is inconsistent with `getStatus()` and se

[PATCH] D48946: [Preamble] Check system dependencies in preamble too

ilya-biryukov added a comment. In https://reviews.llvm.org/D48946#1152764, @sammccall wrote: > This seems like it might be a nontrivial performance hit (it's going to > result in `stat`ing all these files, right?). > Agreed it's important for correctness, it's possible someone wants the > perf

[PATCH] D48946: [Preamble] Check system dependencies in preamble too

ilya-biryukov updated this revision to Diff 154551. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. Herald added a subscriber: omtcyfz. - Fix a comment Repository: rC Clang https://reviews.llvm.org/D48946 Files: lib/Frontend/PrecompiledPreamble.cpp Index: l

[PATCH] D48947: [clangd] Added a test for preambles and -isystem

ilya-biryukov updated this revision to Diff 154552. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Remove Name matcher, use Field(&...::Name) instead Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48947 Files: unittests/clangd/ClangdTests.cpp

[PATCH] D48940: [clangd] Wait for first preamble before code completion

ilya-biryukov updated this revision to Diff 154562. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Add a test Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48940 Files: clangd/TUScheduler.cpp clangd/TUScheduler.h unittests/clangd/TUSched

[PATCH] D48940: [clangd] Wait for first preamble before code completion

ilya-biryukov added a comment. In https://reviews.llvm.org/D48940#1152750, @sammccall wrote: > Possible test: add a file with complicated preamble (billion laughs?) and > immediately schedule 5 preamble actions. They should all get a non-null > preamble and the pointers should all be the same.

[PATCH] D48943: [clangd] Do not write comments into Preamble PCH

ilya-biryukov updated this revision to Diff 154569. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Fix a comment Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48943 Files: clangd/ClangdUnit.cpp clangd/CodeCompletionStrings.cpp Index: cla

[PATCH] D49077: [clangd] Remove JSON library in favor of llvm/Support/JSON

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49077 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

ilya-biryukov added inline comments. Comment at: lib/Basic/FileManager.cpp:320 + SmallString<128> RealPathName; + if (FS->getRealPath(InterndFileName, RealPathName) == std::error_code()) +UFE.RealPathName = RealPathName.str(); NIT: replace replace equality

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM if that does not introduce any regressions in clang and clang-tools. Comment at: lib/Basic/VirtualFileSystem.cpp:770 +if (I != E) { + SmallString<

[PATCH] D49142: [clangd] Extract FileSystemProvider into a separate header. NFC

ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added subscribers: jkorous, MaskRay, ioeric, mgorny. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49142 Files: clangd/CMakeLists.txt clangd/ClangdServer.cpp clangd/ClangdServer.h clan

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

ilya-biryukov added a comment. In https://reviews.llvm.org/D48903#1157600, @simark wrote: > Thanks. I have seen no failures in `check-clang` and `check-clang-tools`, so > I will push it. LG! We can always revert the change is anything breaks... Repository: rC Clang https://reviews.llvm.o

[PATCH] D49175: [clangd] Ignore sema code complete callback with recovery context.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. Thanks! That's a simple fix that makes things better overall! LGTM with a NIT about the comment in the test. Comment at: unittests/clangd/CodeCompleteTests.cpp:797 #if 0 + // In recorvery mode, "param_in_fo

[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:68 +// Lexes from end of \p FD until it finds a semicolon. +llvm::Optional getSemiColonForDecl(const FunctionDecl *FD) { "Lexes" is probably a not very re

[PATCH] D68458: [clangd] Collect missing macro references.

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/D68458/new/ https://reviews.llvm.org/D68458 _

[PATCH] D68467: [clangd] If an undocumented definition exists, don't accept documentation from other forward decls.

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/D68467/new/ https://reviews.llvm.org/D68467 _

[PATCH] D68562: [clangd] Add RemoveUsingNamespace tweak.

ilya-biryukov added a comment. The main comment is about limiting this only to global namespace. PTAL. Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:79 +} +} // namespace + continue rest of the file in the anonymous namespace =

[PATCH] D68565: [clang] Add test for FindNextToken in Lexer.

ilya-biryukov added a comment. Could you add a rationale in the change description? E.g. something like `we do not have any tests for findNextToken` Comment at: clang/unittests/Lex/LexerTest.cpp:559 + } + EXPECT_EQ(ExpectedTokens, GeneratedByNextToken); +} N

[PATCH] D68565: [clang] Add test for FindNextToken in Lexer.

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

[PATCH] D68562: [clangd] Add RemoveUsingNamespace tweak.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:86 + if (const Decl *ParentDecl = Node->Parent->ASTNode.get()) { +return llvm::isa(ParentDecl); + } NIT: remove redundant `{}` around this `r

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

ilya-biryukov added a comment. In D67536#1697825 , @sammccall wrote: > So I don't think clients will/should prefer that - for best rendering they > should know this is a line highlight. I have actually seen clients that just make the text gray and it lo

[PATCH] D68630: [clangd] Disable expand auto on decltype(auto)

ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added subscribers: usaxena95, kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. Applying it produces incorrect code at the moment. Repository: rG LLVM Github Monorepo https://reviews.llvm

[PATCH] D68630: [clangd] Disable expand auto on decltype(auto)

ilya-biryukov added a comment. In D68630#1699371 , @kuhnel wrote: > as the automatic build report did not work: Definitely unrelated, `llvm-ar` does not depend on clangd... I think I was unlucky with the base revision... Repository: rG LLVM Github M

[PATCH] D68630: [clangd] Disable expand auto on decltype(auto)

This revision was automatically updated to reflect the committed changes. Closed by commit rG1b36caf45e5e: [clangd] Disable expand auto on decltype(auto) (authored by ilya-biryukov). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68630/new/ https://r

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

ilya-biryukov added a comment. In D67536#1700872 , @nridge wrote: > > It also lets them consistently highlight part of the line (e.g. dead > > expressions or statements can be marked in gray even if they are on the > > same line). > > Highlighting part o

[PATCH] D64799: [Sema] Emit diagnostics for uncorrected delayed typos at the end of TU

ilya-biryukov added a comment. In D64799#1688293 , @dgoldman wrote: > In D64799#1605514 , @ilya-biryukov > wrote: > > > Tried the suggested approach and ran into the issue described above. > > Either we make the d

[PATCH] D64799: [Sema] Emit diagnostics for uncorrected delayed typos at the end of TU

ilya-biryukov added inline comments. Comment at: clang/lib/Sema/Sema.cpp:916 + // cases in practice. + for (const auto &Typo : DelayedTypos) { +// We pass an empty TypoCorrection to indicate no correction was performed. sammccall wrote: > If you like, `DEBU

[PATCH] D64799: [Sema] Emit diagnostics for uncorrected delayed typos at the end of TU

This revision was automatically updated to reflect the committed changes. Closed by commit rGaeae71cd96c3: [Sema] Emit diagnostics for uncorrected delayed typos at the end of TU (authored by ilya-biryukov). Changed prior to commit: https://reviews.llvm.org/D64799?vs=210338&id=224005#toc Reposi

[PATCH] D68562: [clangd] Add RemoveUsingNamespace tweak.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:87 +return false; + if (const Decl *ParentDecl = Node->Parent->ASTNode.get()) +return llvm::isa(ParentDecl); Can we use `ASTNode.get()` to

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

ilya-biryukov added a comment. Just noticed the next version of LSP added diagnostic tags for things like "unused field" or "dead code": https://microsoft.github.io/language-server-protocol/specifications/specification-3-15, search for `DiagnosticTag`. So I guess we won't need ever add range-ba

[PATCH] D68702: [clangd] Make sure ReplyCallbacks are destroyed before RequestCancelersMutex

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:429 + // for cases where LSP clients don't reply for the request. + static constexpr int MaxReplayCallbacks = 100; + mutable std::mutex CallMutex; Please document tha

[PATCH] D68562: [clangd] Add RemoveUsingNamespace tweak.

ilya-biryukov added a comment. Many thanks, this LG overall, just a few NITs and documentation requests. Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:31 +/// std::vector foo(std::map); +class RemoveUsingNamespace : public Tweak { +public:

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:157 +if (canHighlightName(E->getName())) + addToken(E->getNameLoc(), HighlightingKind::DependentName); +return true; Could we highlighting based on the

[PATCH] D68562: [clangd] Add RemoveUsingNamespace tweak.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM, many thanks! A few last NITs too Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:31 +/// std::vector foo(std::map); +// C

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

ilya-biryukov added a comment. In D67901#1704639 , @nridge wrote: > I like how we went from using heuristics, to not using heuristics, to using > heuristics again :) > > But admittedly, the new heuristics are more accurate because they're based on > phas

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

ilya-biryukov added a comment. LGTM from my side, but not marking as accepted yet to make sure @hokein has a chance to take a final look. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67901/new/ https://reviews.llvm.org/D67901 _

[PATCH] D68978: [clangd] Propagate main context into ClangdServer

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:135 + // Since initialization of CDBs and ClangdServer is done lazily, the following + // context captures the one used while creating ClangdLSPServer and passes it N

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

ilya-biryukov added a comment. Mostly LG, but please take a look at the NITs and the implementation-related comments. Comment at: clang-tools-extra/clangd/FindTarget.cpp:415 Optional refInDecl(const Decl *D) { struct Visitor : ConstDeclVisitor { This sho

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

ilya-biryukov added a comment. An alternative approach I'm thinking of: After D68977 lands, we could try using `findExplicitReferences` to produce all of these edits: 1. we collect locations of all references and declaration of relevant parameters and template

[PATCH] D56612: [clangd] A code action to remove 'using namespace'

ilya-biryukov abandoned this revision. ilya-biryukov added a comment. This was finalized and landed as D68562 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56612/new/ https://reviews.llvm.org/D56612 ___ cfe

[PATCH] D67827: [clangd] Simplify name qualification in DefineInline

ilya-biryukov abandoned this revision. ilya-biryukov added a comment. D66647 was updated to use `findExplicitReferences`, abandoning this revision. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67827/new/ https://

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

ilya-biryukov added a comment. We seem to have trouble only in presence of using declarations and using namespace directives. I wonder how complicated it would be to take them into account instead. That would clearly be easier to read, as we'll hit right into the center of the problem. Could y

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:528 + /// AST. + AllRefs annotateReferencesInMainAST(llvm::StringRef Code) { +TestTU TU; hokein wrote: > ilya-biryukov wrote: > > Why do we need this? Ca

[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 { hokein wrote: > ilya-biryukov wrote: > > This should return `SmallVector` now, some declarat

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:846 + // the underlying FunctionDecl. we should report just one. + void $12^$13^F() {} + )cpp", Could we avoid adding this te

[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** a

[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 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(), + D->getIdentLo

[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 `VisitE

[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] 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 https://reviews.llvm.

[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 a

[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 class?

[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) { + visitNode(DynTypedN

[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". There

[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] 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 &AST, unsigned Ra

[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 con

[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] 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 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()); - assert(CCI->isAnyMemberInitializer())

[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()); - assert(CCI->isAnyMemberInitializer())

[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 { +SkippedRanges.push_ba

[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 affected

[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 time

[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 expres

[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 `ActionWithCtx`

[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 t

[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] 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] 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 &SM = FD->getASTContext().ge

[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 don't

[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 &RenameDecl, const SymbolIndex *Index) { hokein wrote: > ilya-biryukov wrote: > > So `llvm::None` means we d

[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] 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: > > NI

[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] 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 chu

<    17   18   19   20   21   22   23   24   25   26   >