[PATCH] D57509: [clangd] Append "(fix available)" to diagnostic message when fixes are present.

ioeric added a comment. In D57509#1378707 , @sammccall wrote: > In D57509#1378696 , @kadircet wrote: > > > Dropping by: Maybe add `(fix available)` as a prefix rather than suffix. > > Since long texts might end up

[PATCH] D57509: [clangd] Append "(fix available)" to diagnostic message when fixes are present.

This revision was automatically updated to reflect the committed changes. Closed by commit rL352764: [clangd] Append "(fix available)" to diagnostic message when fixes are present. (authored by ioeric, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM CHANGES SINCE L

[PATCH] D57509: [clangd] Append "(fix available)" to diagnostic message when fixes are present.

ioeric added a comment. In D57509#1378928 , @ilya-biryukov wrote: > Could this be made optional for VSCode? > As mentioned in the discussion before, I would personally prefer to turn it > off, even though I do acknowledge the need for this for some clie

[PATCH] D57509: [clangd] Append "(fix available)" to diagnostic message when fixes are present.

ioeric added a subscriber: hokein. ioeric added a comment. In D57509#1378978 , @ilya-biryukov wrote: > In D57509#1378943 , @ioeric wrote: > > > Yes, but a new option seems a bit of an overkill here. The text is >

[PATCH] D53926: [clangd] Only add global scope to completion query scopes for TU context.

ioeric added inline comments. Comment at: clangd/CodeComplete.cpp:563 for (auto *Context : CCContext.getVisitedContexts()) { - if (isa(Context)) + if (isa(Context)) { Info.AccessibleScopes.push_back(""); // global namespace ilya-biryukov w

[PATCH] D53926: [clangd] Only add global scope to completion query scopes for TU context.

ioeric updated this revision to Diff 171924. ioeric added a comment. - Clarify documentation for printNamespaceScope Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53926 Files: clangd/AST.h clangd/CodeComplete.cpp unittests/clangd/CodeCompleteTests.cpp Index: unittests/

[PATCH] D53926: [clangd] Only add global scope to completion query scopes for TU context.

ioeric updated this revision to Diff 171928. ioeric added a comment. - revert wrong comment Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53926 Files: clangd/AST.h clangd/CodeComplete.cpp unittests/clangd/CodeCompleteTests.cpp Index: unittests/clangd/CodeCompleteTests.

[PATCH] D53926: [clangd] Only add global scope to completion query scopes for TU context.

ioeric planned changes to this revision. ioeric added a comment. I got the behavior of `printNamespaceScope` wrong in this patch. Will update. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53926 ___ cfe-commits mailing list cfe-com

[PATCH] D54092: [Tooling] Add "-filter" option to AllTUsExecution

ioeric added inline comments. Comment at: lib/Tooling/AllTUsExecution.cpp:58 +"filter", +llvm::cl::desc("Only process files that match this filter"), +llvm::cl::init(".*")); Please also mention that this only applies to all-TUs. Com

[PATCH] D54092: [Tooling] Add "-filter" option to AllTUsExecution

ioeric added inline comments. Comment at: lib/Tooling/AllTUsExecution.cpp:120 for (std::string File : Files) { + if (Filter.getNumOccurrences() != 0 && !RegexFilter.match(File)) +continue; hokein wrote: > ioeric wrote: > > > `Filter.getNumOccurr

[PATCH] D54092: [Tooling] Add "-filter" option to AllTUsExecution

ioeric accepted this revision. ioeric added inline comments. This revision is now accepted and ready to land. Comment at: lib/Tooling/AllTUsExecution.cpp:120 for (std::string File : Files) { + if (Filter.getNumOccurrences() != 0 && !RegexFilter.match(File)) +con

[PATCH] D54092: [Tooling] Add "-filter" option to AllTUsExecution

ioeric added a comment. maybe add a test? Repository: rC Clang https://reviews.llvm.org/D54092 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54104: [Tooling] Correct the total number of files being processed when `filter` is provided.

ioeric accepted this revision. ioeric added inline comments. This revision is now accepted and ready to land. Comment at: lib/Tooling/AllTUsExecution.cpp:103 +if (!RegexFilter.match(File)) + continue; +Files.push_back(File); nit: `if (match) then pus

[PATCH] D54105: [clangd] Deduplicate query scopes.

ioeric created this revision. ioeric added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay. For example, when anonymous namespace is present, duplicated namespaces might be generated for the enclosing namespace. Repository: rCTE Clang Tool

[PATCH] D53926: [clangd] Only add global scope to completion query scopes for TU context.

ioeric abandoned this revision. ioeric marked an inline comment as done. ioeric added inline comments. Comment at: clangd/CodeComplete.cpp:563 for (auto *Context : CCContext.getVisitedContexts()) { - if (isa(Context)) + if (isa(Context)) { Info.AccessibleS

[PATCH] D53933: [clangd] Get rid of QueryScopes.empty() == AnyScope special case.

ioeric updated this revision to Diff 172591. ioeric marked an inline comment as done. ioeric added a comment. - Remove addressed FIXME. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53933 Files: clangd/FindSymbols.cpp clangd/index/Index.h clangd/index/MemIndex.cpp clan

[PATCH] D54106: [clangd] Limit the index-returned results in dexp.

ioeric added inline comments. Comment at: clangd/index/dex/dexp/Dexp.cpp:128 }; cl::opt Limit{ "limit", I think we should make it configurable like what we do here. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54106 __

[PATCH] D53433: [clangd] auto-index stores symbols per-file instead of per-TU.

ioeric added inline comments. Comment at: clangd/index/Background.cpp:235 +IndexedFileDigests[Path] = FilesToUpdate.lookup(Path); +IndexedSymbols.update(Path, + make_unique(std::move(Syms).build()), kadircet wrote: > This call is

[PATCH] D54077: [clangd] Implemented DraftFileSystem

ioeric added a comment. In https://reviews.llvm.org/D54077#1288387, @sammccall wrote: > Someone mentioned to me that the interaction-between-features argument wasn't > clear here: > > - we **don't** currently update diagnostics for `A.cc` when `A.h` is edited > - we should, this seems more obvio

[PATCH] D53433: [clangd] auto-index stores symbols per-file instead of per-TU.

ioeric updated this revision to Diff 172732. ioeric added a comment. - Rebase Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53433 Files: clangd/index/Background.cpp clangd/index/Background.h clangd/index/FileIndex.cpp clangd/index/FileIndex.h clangd/index/SymbolColle

[PATCH] D53433: [clangd] auto-index stores symbols per-file instead of per-TU.

This revision was automatically updated to reflect the committed changes. Closed by commit rL346221: [clangd] auto-index stores symbols per-file instead of per-TU. (authored by ioeric, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D53433

[PATCH] D53933: [clangd] Get rid of QueryScopes.empty() == AnyScope special case.

ioeric updated this revision to Diff 172735. ioeric added a comment. - rebase Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53933 Files: clangd/FindSymbols.cpp clangd/index/Index.h clangd/index/MemIndex.cpp clangd/index/dex/Dex.cpp unittests/clangd/DexTests.cpp uni

[PATCH] D53933: [clangd] Get rid of QueryScopes.empty() == AnyScope special case.

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE346223: [clangd] Get rid of QueryScopes.empty() == AnyScope special case. (authored by ioeric, committed by ). Changed prior to commit: https://reviews.llvm.org/D53933?vs=172735&id=172737#toc Reposit

[PATCH] D54106: [clangd] Limit the index-returned results in dexp.

ioeric added inline comments. Comment at: clangd/index/dex/dexp/Dexp.cpp:166 + cl::init(10), + cl::desc("Max results to display. This flag is only meaningful when -name" + " is set."), Maybe `The max number of symbols with the same name b

[PATCH] D54105: [clangd] Deduplicate query scopes.

This revision was automatically updated to reflect the committed changes. Closed by commit rL346224: [clangd] Deduplicate query scopes. (authored by ioeric, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D54105 Files: clang-tools-extra/

[PATCH] D54300: [clangd] Respect shouldIndexFile when collecting symbols.

ioeric added inline comments. Comment at: clangd/index/SymbolCollector.cpp:555 + auto Loc = findNameLoc(&ND); + if (!shouldIndexFile(SM, SM.getFileID(Loc), Opts, &FilesToIndexCache)) +return nullptr; Should we use `getTokenLocation` like what we do below?

[PATCH] D54300: [clangd] Respect shouldIndexFile when collecting symbols.

ioeric added inline comments. Comment at: clangd/index/SymbolCollector.cpp:217 +bool shouldIndexFile(const Decl& D, const SymbolCollector::Options &Opts, + llvm::DenseMap *FilesToIndexCache) { nit: this is very easily confused with the callb

[PATCH] D52273: [clangd] Initial implementation of expected types

ioeric added inline comments. Comment at: clangd/ExpectedTypes.cpp:27 +return llvm::None; + auto *VD = llvm::dyn_cast(R.Declaration); + if (!VD) maybe add a comment what `ValueDecl` covers roughly? E.g. functions, classes, variables etc.

[PATCH] D54427: [clangd] Allow symbols from AnyScope in dexp.

ioeric added inline comments. Comment at: clangd/index/dex/dexp/Dexp.cpp:55 FuzzyFindRequest Request; + Request.AnyScope = true; // Remove leading "::" qualifier as FuzzyFind doesn't need leading "::" I don't think you would want AnyScope here. For example

[PATCH] D54309: [AST] Allow limiting the scope of common AST traversals (getParents, RAV).

ioeric added a subscriber: thakis. ioeric added a comment. The new API and the refactoring look good to me. Just a nit and a question. Comment at: lib/AST/ASTContext.cpp:10292 + if (!Parents) // We always need to run over the whole translation unit, as // hasAncestor

[PATCH] D54309: [AST] Allow limiting the scope of common AST traversals (getParents, RAV).

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lgtm Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:674 + // Nodes may have no parents if: + // a) the node is the TranslationUnitDecl + // a) there is a bug

[PATCH] D54519: [clangd] Fix no results returned for global symbols in dexp

ioeric added inline comments. Comment at: clangd/index/dex/dexp/Dexp.cpp:64 +// add the global scope to the request. +Request.Scopes = {""}; I think the old behavior used `AnyScope` for a unqualified name. Do we want to keep that? Repository: rCTE

[PATCH] D54519: [clangd] Fix no results returned for global symbols in dexp

ioeric accepted this revision. ioeric added inline comments. This revision is now accepted and ready to land. Comment at: clangd/index/dex/dexp/Dexp.cpp:64 +// add the global scope to the request. +Request.Scopes = {""}; ioeric wrote: > I think the old

[PATCH] D54800: [clangd] Cleanup: stop passing around list of supported URI schemes.

ioeric created this revision. ioeric added a reviewer: sammccall. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. Instead of passing around a list of supported URI schemes in clangd, we expose an interface to convert a path to URI using any compatible s

[PATCH] D54800: [clangd] Cleanup: stop passing around list of supported URI schemes.

ioeric updated this revision to Diff 174929. ioeric added a comment. - remove unused include Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54800 Files: clangd/ClangdLSPServer.cpp clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/URI.cpp clangd/URI.h clangd/index

[PATCH] D54781: [clangd] Add 'Switch header/source' command in clangd-vscode

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lgtm Could you run clang-format on the changed lines? Comment at: clangd/clients/clangd-vscode/package.json:81 +"command": "clangd-vscode.switchheadersource"

[PATCH] D54833: [clangd] Cleanup: use index file instead of header in workspace symbols lit test.

ioeric created this revision. ioeric added a reviewer: sammccall. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. The full path of the input header depends on the execution environment and may result in different behavior (e.g. when different URI scheme

[PATCH] D54800: [clangd] Cleanup: stop passing around list of supported URI schemes.

ioeric updated this revision to Diff 175048. ioeric marked 3 inline comments as done. ioeric added a comment. - address review comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54800 Files: clangd/ClangdLSPServer.cpp clangd/ClangdServer.cpp clangd/ClangdServer.h c

[PATCH] D54833: [clangd] Cleanup: use index file instead of header in workspace symbols lit test.

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE347466: [clangd] Cleanup: use index file instead of header in workspace symbols lit… (authored by ioeric, committed by ). Changed prior to commit: https://reviews.llvm.org/D54833?vs=175047&id=175051#t

[PATCH] D54800: [clangd] Cleanup: stop passing around list of supported URI schemes.

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE347467: [clangd] Cleanup: stop passing around list of supported URI schemes. (authored by ioeric, committed by ). Changed prior to commit: https://reviews.llvm.org/D54800?vs=175048&id=175052#toc Repo

[PATCH] D54845: [clangd] Canonicalize file path in URIForFile.

ioeric created this revision. ioeric added a reviewer: sammccall. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. File paths in URIForFile can come from index or local AST. Path from index goes through URI transformation and the final path is resolved b

[PATCH] D54851: [clangd] Tune down scope boost for global scope

ioeric created this revision. ioeric added a reviewer: sammccall. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. This improves cross-namespace completions and has ignorable impact on other completion types. Metrics === OVERALL (excl. CROSS_NAME

[PATCH] D54851: [clangd] Tune down scope boost for global scope

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE347548: [clangd] Tune down scope boost for global scope (authored by ioeric, committed by ). Changed prior to commit: https://reviews.llvm.org/D54851?vs=175106&id=175226#toc Repository: rCTE Clang

[PATCH] D54300: [clangd] Respect shouldIndexFile when collecting symbols.

ioeric added inline comments. Comment at: clangd/index/SymbolCollector.cpp:386 const Symbol *BasicSymbol = Symbols.find(*ID); - if (!BasicSymbol) // Regardless of role, ND is the canonical declaration. -BasicSymbol = addDeclaration(*ND, std::move(*ID)); - else if (isPref

[PATCH] D54894: [clangd] Enable auto-index behind a flag.

ioeric added a comment. lgtm (will let @kadircet stamp ) Comment at: test/clangd/background-index.test:19 +# Test the index is read from disk: delete code and restart clangd. +# This test currently fails as we don't read the index yet. +# RUN: rm %t/foo.cpp May

[PATCH] D54845: [clangd] Canonicalize file path in URIForFile.

ioeric updated this revision to Diff 175466. ioeric marked 7 inline comments as done. ioeric added a comment. address comments and rebase Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54845/new/ https://reviews.llvm.org/D54845 Files: clangd/Clang

[PATCH] D54845: [clangd] Canonicalize file path in URIForFile.

ioeric added a comment. Thanks for the review! PTAL Comment at: clangd/Protocol.h:74 + /// \p AbsPath is resolved to a canonical path corresponding to its URI. + URIForFile(llvm::StringRef AbsPath, llvm::StringRef HintPath = ""); + sammccall wrote: > sammccal

[PATCH] D54845: [clangd] Canonicalize file path in URIForFile.

ioeric added a comment. In D54845#1309589 , @sammccall wrote: > Mostly just places that should be updated HintPath -> TUPath. Changing the > whole codebase doesn't seem important, but in places that are touched... As chatted offline, `URIForFile` is clo

[PATCH] D54845: [clangd] Canonicalize file path in URIForFile.

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE347739: [clangd] Canonicalize file path in URIForFile. (authored by ioeric, committed by ). Changed prior to commit: https://reviews.llvm.org/D54845?vs=175466&id=175642#toc Repository: rCTE Clang T

[PATCH] D55052: Fix junk output in clangd vscode plugin

ioeric added a comment. Thanks for the patch James! Adding folks who are currently using vscode+clangd to get more opinions. Comment at: clangd/clients/clangd-vscode/src/extension.ts:59 +// Avoid lots of junk in output +revealOutputChannelOn: vscodelc.RevealOut

[PATCH] D55052: Fix junk output in clangd vscode plugin

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. In D55052#1312766 , @hokein wrote: > In D55052#1312760 , @ilya-biryukov > wrote: > > > +1 to the change, this

[PATCH] D55061: [clangd] Penalize destructor and overloaded operators in code completion.

ioeric created this revision. ioeric added a reviewer: hokein. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D55061 Files: clangd/Quality.cpp clangd/Quality.h unittests/clangd/Quali

[PATCH] D55065: [clangd] Drop injected class name when class scope is not explicitly specified.

ioeric created this revision. ioeric added a reviewer: kadircet. Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, ilya-biryukov. E.g. allow injected "A::A" in `using A::A^` but not in "A^". Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D55065 Files: clangd/

[PATCH] D55052: Fix junk output in clangd vscode plugin

ioeric added a comment. James, do you have commit access to llvm? If not, I'm happy to land the patch for you. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55052/new/ https://reviews.llvm.org/D55052 ___ cfe-commits mailing list cfe-commit

[PATCH] D55065: [clangd] Drop injected class name when class scope is not explicitly specified.

ioeric marked an inline comment as done. ioeric added inline comments. Comment at: unittests/clangd/CodeCompleteTests.cpp:419 +TEST(CompletionTest, SkipInjectedWhenUnqualified) { + EXPECT_THAT(completions("struct X { void f() { X^ }};").Completions, kadircet w

[PATCH] D55065: [clangd] Drop injected class name when class scope is not explicitly specified.

This revision was automatically updated to reflect the committed changes. Closed by commit rL347982: [clangd] Drop injected class name when class scope is not explicitly specified. (authored by ioeric, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM CHANGES SINCE L

[PATCH] D55061: [clangd] Penalize destructor and overloaded operators in code completion.

ioeric updated this revision to Diff 176075. ioeric marked an inline comment as done. ioeric added a comment. - Rename test name. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55061/new/ https://reviews.llvm.org/D55061 Files: clangd/Quality.cpp

[PATCH] D55061: [clangd] Penalize destructor and overloaded operators in code completion.

ioeric added inline comments. Comment at: clangd/Quality.h:74 Keyword, +Operator, } Category = Unknown; hokein wrote: > Maybe name it `OverloadedOperator` to avoid confusion with other > non-overloaded operators like `?:` Ternary operator is not a sy

[PATCH] D55061: [clangd] Penalize destructor and overloaded operators in code completion.

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE347983: [clangd] Penalize destructor and overloaded operators in code completion. (authored by ioeric, committed by ). Changed prior to commit: https://reviews.llvm.org/D55061?vs=176075&id=176076#toc

[PATCH] D58448: [clangd] Improve global code completion when scope specifier is unresolved.

ioeric updated this revision to Diff 188512. ioeric added a comment. - Rebase and minor cleanup Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58448/new/ https://reviews.llvm.org/D58448 Files: clangd/CodeComplete.cpp unittests/clangd/CodeComplet

[PATCH] D58448: [clangd] Improve global code completion when scope specifier is unresolved.

This revision was automatically updated to reflect the committed changes. Closed by commit rL354963: [clangd] Improve global code completion when scope specifier is unresolved. (authored by ioeric, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Repository:

[PATCH] D58768: Moved SymbolLocation into its own header and implementation file

ioeric added a comment. I think another option here is to move Symbol+SymbolLocations to a Symbol.h library. SymbolLocation is often used along with Symbol. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58768/new/ https://reviews.llvm.org/D58768

[PATCH] D56830: Prototype for include-fixer workflow in clangd. [NOT FOR REVIEW]

ioeric abandoned this revision. ioeric added a comment. Herald added a subscriber: jdoerfert. Herald added a project: clang. Include-fixer has been landed in clangd. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56830/new/ https://reviews.llvm.org/D

[PATCH] D58772: [clangd] Enable SuggestMissingIncludes by default.

ioeric created this revision. ioeric added a reviewer: kadircet. Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, ilya-biryukov. Herald added a project: clang. This seems to work stably now. Turn on by default. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D58

[PATCH] D58773: Moved SymbolOrigin into its own header and implementation file

ioeric added a comment. I'm not sure if SymbolOrigin is interesting enough to be its own library. It's usually only used along with Symbol. Maybe we could pull a library Symbol.h instead and put SymbolOrigin by Symbol? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://

[PATCH] D58773: Moved SymbolOrigin into its own header and implementation file

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. In D58773#1413486 , @gribozavr wrote: > SymbolOrigin is used by itself, for example, in CodeComplete.h to define > `struct CodeCompletion`. Fair enou

[PATCH] D58774: Moved Symbol into its own header and implementation file

ioeric added a comment. The design of Symbol and SymbolSlab are closely related. I would suggest putting them close together in the same library. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58774/new/ https://reviews.llvm.org/D58774 _

[PATCH] D58774: Moved Symbol into its own header and implementation file

ioeric added inline comments. Comment at: clang-tools-extra/clangd/index/Index.cpp:19 -llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, Symbol::SymbolFlag F) { - if (F == Symbol::None) -return OS << "None"; - std::string S; - if (F & Symbol::Deprecated) -S += "de

[PATCH] D58778: Moved Ref into its own header and implementation file

ioeric added inline comments. Comment at: clang-tools-extra/clangd/index/Ref.cpp:1 +#include "Ref.h" + License? Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:14 #include "CodeCompletionStrings.h" +#include "ExpectedTypes.h" #

[PATCH] D58781: Added missing license headers

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lg. thanks!!! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58781/new/ https://reviews.llvm.org/D58781 __

[PATCH] D58782: Use ArrayRef::copy, instead of copying data manually

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

[PATCH] D58772: [clangd] Enable SuggestMissingIncludes by default.

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE355200: [clangd] Enable SuggestMissingIncludes by default. (authored by ioeric, committed by ). Changed prior to commit: https://reviews.llvm.org/D58772?vs=188701&id=188894#toc Repository: rCTE Cla

[PATCH] D58977: [clang-tidy] Add the abseil-time-comparison check

ioeric added inline comments. Comment at: clang-tidy/abseil/TimeComparisonCheck.cpp:23 + auto Matcher = + binaryOperator(anyOf(hasOperatorName(">"), hasOperatorName(">="), + hasOperatorName("=="), hasOperatorName("<="), `DurationCo

[PATCH] D59083: [clangd] Store explicit template specializations in index for code navigation purposes

ioeric added a comment. > We need that information for providing better code navigation support, like > find references, children/base classes etc. It's not trivial how they make code navigation better. Can you explain and provide examples in the summary? Thanks! Comment at:

[PATCH] D59086: [clangd] Adjust compile commands to be applicable for tooling

ioeric added inline comments. Comment at: clangd/GlobalCompilationDatabase.h:116 + /// Adjusts given compile command for clangd. + tooling::CompileCommand adjustArguments(tooling::CompileCommand Cmd) const; + This doesn't seem to be used in this patch (except f

[PATCH] D59086: [clangd] Adjust compile commands to be applicable for tooling

ioeric added inline comments. Comment at: clangd/GlobalCompilationDatabase.cpp:25 + // Clangd does not generate dependency file. + Cmd.CommandLine = tooling::getClangStripDependencyFileAdjuster()( + Cmd.CommandLine, Cmd.Filename); Please use `clang::toolin

[PATCH] D58977: [clang-tidy] Add the abseil-time-comparison check

ioeric accepted this revision. ioeric added inline comments. This revision is now accepted and ready to land. Comment at: clang-tidy/abseil/TimeComparisonCheck.cpp:23 + auto Matcher = + binaryOperator(anyOf(hasOperatorName(">"), hasOperatorName(">="), +

[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

ioeric added inline comments. Comment at: clangd/StdGen/StdGen.py:2 +#!/usr/bin/env python +#===- StdGen.py - ---*- python -*--===# +# I'd avoid abbreviation in the file name and the new directory name. `StdGen` sounds a

[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

ioeric added inline comments. Comment at: clangd/StdGen/StdGen.py:2 +#!/usr/bin/env python +#===- StdGen.py - ---*- python -*--===# +# hokein wrote: > ioeric wrote: > > I'd avoid abbreviation in the file name and the new

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

ioeric added a comment. just a few drive-by comments ;) Comment at: clang-tools-extra/clangd/FindSymbols.h:28 +// https://github.com/Microsoft/language-server-protocol/issues/344 +SymbolKind indexSymbolKindToSymbolKind(index::SymbolKind Kind); + This could prob

[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

ioeric added inline comments. Comment at: clangd/include-mapping/gen_std.py:46 +// +// Generated from cppreference offline HTML book v%s. +//===--===// I'd suggest dropping `v` prefix as it might

[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

ioeric accepted this revision. ioeric added inline comments. This revision is now accepted and ready to land. Comment at: clangd/index/CanonicalIncludes.cpp:111 - {"std::addressof", ""}, - // Map symbols in to their preferred includes. {"std::basic_filebuf", ""}

[PATCH] D59300: [clangd] Tune the fuzzy-matching algorithm

ioeric added inline comments. Comment at: clang-tools-extra/clangd/FuzzyMatch.cpp:74 static bool isAwful(int S) { return S < AwfulScore / 2; } -static constexpr int PerfectBonus = 3; // Perfect per-pattern-char score. +static constexpr int PerfectBonus = 4; // Perfect per-patter

[PATCH] D59300: [clangd] Tune the fuzzy-matching algorithm

ioeric added inline comments. Comment at: clang-tools-extra/clangd/FuzzyMatch.cpp:285 + if (Pat[P] == Word[W] || + (WordRole[W] == Head && (IsPatSingleCase || PatRole[P] == Head))) ++S; ilya-biryukov wrote: > ioeric wrote: > > could you explain the int

[PATCH] D59300: [clangd] Tune the fuzzy-matching algorithm

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. (The result looks great) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59300/new/ https://reviews.llvm.org/D59300 ___

[PATCH] D59481: [clangd] Count number of references while merging RefSlabs inside FileIndex

ioeric added a comment. I'm not sure if FileIndex is the correct layer to make decision about how References is calculated. Currently, we have two use cases in clangd 1) one slab per TU and 2) one slab per file. It seems to me that we would want different strategies for the two use cases, so it

[PATCH] D59487: [Tooling] Add more scope specifiers until spelling is not ambiguous.

ioeric created this revision. ioeric added reviewers: kadircet, gribozavr. Herald added subscribers: cfe-commits, jdoerfert. Herald added a project: clang. Previously, when the renamed spelling is ambiguous, we simply use the full-qualfied name (with leading "::"). This patch makes it try adding a

[PATCH] D59487: [Tooling] Add more scope specifiers until spelling is not ambiguous.

ioeric marked 2 inline comments as done. ioeric added inline comments. Comment at: lib/Tooling/Core/Lookup.cpp:165 +if (UnspelledScopes.empty()) { + Disambiguated = "::" + Disambiguated; +} else { kadircet wrote: > maybe return or break afterwards? I

[PATCH] D59481: [clangd] Count number of references while merging RefSlabs inside FileIndex

ioeric added a comment. > I don't follow why this can over-count, FileIndex keeps only one RefSlab per > each file, so we can't over-count? Did you mean it will be more than #TUs ? Yes. This is how `Symbol::References` is described in its documentation. If we want to change/expand the semantic,

[PATCH] D59487: [Tooling] Add more scope specifiers until spelling is not ambiguous.

This revision was automatically updated to reflect the committed changes. ioeric marked an inline comment as done. Closed by commit rL356446: [Tooling] Add more scope specifiers until spelling is not ambiguous. (authored by ioeric, committed by ). Herald added a project: LLVM. Herald added a subsc

[PATCH] D59481: [clangd] Count number of references while merging RefSlabs inside FileIndex

ioeric added inline comments. Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:33 +// If MergedSyms is provided, increments a symbols Reference count once for +// every file it was mentioned in. +void mergeRefs(llvm::ArrayRef> RefSlabs, nit: s/every file/

[PATCH] D59639: [clangd] Print template arguments helper

ioeric added inline comments. Comment at: clang-tools-extra/clangd/AST.cpp:88 static const TemplateArgumentList * getTemplateSpecializationArgs(const NamedDecl &ND) { if (auto *Func = llvm::dyn_cast(&ND)) can we unify this with `getTemplateSpecializationArgL

[PATCH] D59640: [clangd] Add TemplateArgumentList into Symbol

ioeric added a comment. should we update YAML? Comment at: clang-tools-extra/clangd/index/Symbol.h:48 + /// non-specializations. Example: "" + llvm::StringRef TemplateArgumentList; /// The location of the symbol's definition, if one was found. How about `

[PATCH] D59639: [clangd] Print template arguments helper

ioeric added inline comments. Comment at: clang/lib/AST/TypePrinter.cpp:1640 + +static void printArgument(const TemplateArgumentLoc &A, + const PrintingPolicy &PP, llvm::raw_ostream &OS) { It's unclear to me what the new behavior is with

[PATCH] D59683: [Tooling] Avoid working-dir races in AllTUsToolExecutor

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

[PATCH] D59639: [clangd] Print template arguments helper

ioeric added inline comments. Comment at: clang-tools-extra/clangd/AST.cpp:149 +} else { + // FIXME: Fix cases when getTypeAsWritten returns null, e.g. friend decls. + printTemplateArgumentList(OS, Cls->getTemplateArgs().asArray(), Policy); I'm not

[PATCH] D59700: Make clang-move use same file naming convention as other tools

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lgtm CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59700/new/ https://reviews.llvm.org/D59700 ___ cfe-commits mailing list cfe-commits@l

[PATCH] D59811: [clangd] Add fallback mode for code completion when compile command or preamble is not ready.

ioeric created this revision. ioeric added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, jdoerfert, kadircet, arphaman, jkorous, MaskRay, javed.absar. Herald added a project: clang. When calling TUScehduler::runWithPreamble (e.g. in code compleiton), allow entering a fallback

[PATCH] D59811: [clangd] Add fallback mode for code completion when compile command or preamble is not ready.

ioeric updated this revision to Diff 192683. ioeric marked 9 inline comments as done. ioeric added a comment. - address review comments. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59811/new/ https://reviews.llvm.org/D59811 Files: clangd/Clangd

[PATCH] D59811: [clangd] Add fallback mode for code completion when compile command or preamble is not ready.

ioeric added a comment. In D59811#1445701 , @ilya-biryukov wrote: > I think this option should be configurable (and off by default) for the > transition period. A few reasons to do so: > > - Before we have an actual implementation of fallback completions

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