[PATCH] D67654: [clang-tidy] Fix a potential infinite loop in readability-isolate-declaration check.

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clang-tidy/utils/LexerUtils.h:38 template SourceLocation findPreviousAnyTokenKind(SourceLocation Start, const

[PATCH] D67584: [Support] Replace function with function_ref in writeFileAtomically. NFC

ilya-biryukov reopened this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. Ah, the tests fail to compile because we now have an overload where the only difference is `function_ref` vs `StringRef`. This works fine with `std::function`, STL probably

[PATCH] D67654: [clang-tidy] Fix a potential infinite loop in readability-isolate-declaration check.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. The fix itself LGTM, getting rid of an infinite loop is important. All my comment can be addressed later... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D67654: [clang-tidy] Fix a potential infinite loop in readability-isolate-declaration check.

ilya-biryukov added a comment. Do we have unit tests for clang-tidy? If yes, could we also add unit-tests for this function? The modified function only needs a source manager and lang options, these are easy to mock. Comment at:

[PATCH] D67607: [clangd] Fix a crash when renaming operator.

ilya-biryukov added a comment. LGTM to fix the crash. See the comment about more cases that could potentially break, though. Do we handle them gracefully now? Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:77 return ReasonToReject::UnsupportedSymbol; + if

[PATCH] D67341: [clangd] Simplify semantic highlighting visitor

ilya-biryukov added a comment. @hokein, do you want to do another round or is this good to go? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67341/new/ https://reviews.llvm.org/D67341 ___ cfe-commits

[PATCH] D67584: [Support] Replace function with function_ref in writeFileAtomically. NFC

ilya-biryukov updated this revision to Diff 220209. ilya-biryukov added a comment. - Reformat Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67584/new/ https://reviews.llvm.org/D67584 Files: llvm/include/llvm/Support/FileUtilities.h

[PATCH] D67584: [Support] Replace function with function_ref in writeFileAtomically. NFC

ilya-biryukov created this revision. ilya-biryukov added a reviewer: jkorous. Herald added subscribers: llvm-commits, jfb, dexonsmith, hiraditya. Herald added a project: LLVM. The latter is slightly more efficient and communicates the intent of the API: writeFileAtomically does not own or copy

[PATCH] D67341: [clangd] Simplify semantic highlighting visitor

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:169 private: - void addTokenForTypedef(SourceLocation Loc, const TypedefNameDecl *TD) { -auto *TSI = TD->getTypeSourceInfo(); -if (!TSI) - return; -// Try to

[PATCH] D67341: [clangd] Simplify semantic highlighting visitor

ilya-biryukov updated this revision to Diff 219503. ilya-biryukov marked 6 inline comments as done. ilya-biryukov added a comment. - Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67341/new/ https://reviews.llvm.org/D67341 Files:

[PATCH] D67264: [clangd] Collect location of macro definition in the ParsedAST

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. We should probably also take a look at highlighting macros inside the preamble part of the main file. @hokein, are you planning to do this or should we just file a bug for

[PATCH] D67341: [clangd] Simplify semantic highlighting visitor

ilya-biryukov added a comment. In D67341#1663104 , @hokein wrote: > Unfortunately, the patch is a bit large, containing refactoring changes and > functionality changes, is it possible to split it into (two) smaller patches? Done. There should be no

[PATCH] D67341: [clangd] Simplify semantic highlighting visitor

ilya-biryukov updated this revision to Diff 219363. ilya-biryukov added a comment. - Turn into NFC, do not highlight lambdas differently Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67341/new/ https://reviews.llvm.org/D67341 Files:

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. Thanks for all suggestions. The final result is rather small and minimal. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67172/new/ https://reviews.llvm.org/D67172 ___

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov updated this revision to Diff 219360. ilya-biryukov marked 3 inline comments as done. ilya-biryukov added a comment. - Make MaxSuffixComponents a constant - Put the suffix mapping into a single constant - Initialize all StringMaps directly - Rename to Std...Mapping Repository:

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov marked 8 inline comments as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:41 + if (!SuffixHeaderMapping) +return Header; sammccall wrote: > nit: can we write `if

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. In D67172#1663096 , @sammccall wrote: > LG, though if we can drop the struct and make MaxSuffixComponents a constant > it'd be simpler still. Done. > Sure. This is going to win a couple of percent I guess: for these

[PATCH] D67290: [clangd] Add a new highlighting kind for typedefs

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:530 + case HighlightingKind::Typedef: +return "entity.name.type.typedef.cpp"; case HighlightingKind::Namespace: hokein wrote: > ilya-biryukov wrote: > >

[PATCH] D67290: [clangd] Add a new highlighting kind for typedefs

ilya-biryukov updated this revision to Diff 219348. ilya-biryukov marked 3 inline comments as done. ilya-biryukov added a comment. - Remove stale comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67290/new/ https://reviews.llvm.org/D67290

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:748 +llvm::StringRef CanonicalPath = Pair.second; +int Components = std::distance(llvm::sys::path::begin(Suffix), +

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. The new version looks very much like the older one, with a few additions to not re-initialize data twice. PTAL and let me know what you think. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67172/new/

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov updated this revision to Diff 219335. ilya-biryukov added a comment. - Alternative approach - do not add extra classes, aim to minimize changes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67172/new/ https://reviews.llvm.org/D67172

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. In D67172#1662945 , @sammccall wrote: > Only if it's 5% of something meaningful. If the denominator isn't something > we care about, then it's really "spending XXX usec is not ok" - which depends > on XXX I think. Agree,

[PATCH] D67264: [clangd] Collect location of macro definition in the ParsedAST

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ParsedAST.cpp:104 // *definitions* in the preamble region of the main file). class CollectMainFileMacroExpansions : public PPCallbacks { const SourceManager hokein wrote: > The name

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. In D67172#1662888 , @sammccall wrote: > I do wonder whether we're microoptimizing for the tests too much, I don't > think 5% on the tests is worth very much in itself, unless it's speeding up > real workloads or improving

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:780 + // Prefer a mapping from the system symbols. + if (SystemSymbols) { +if (auto Result = SystemSymbols->mapHeader(Header, QualifiedName)) hokein

[PATCH] D66516: [clangd] Highlight typedefs to template parameters as template parameters

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:129 if (const auto *TSI = TD->getTypeSourceInfo()) - addType(TD->getLocation(), TSI->getTypeLoc().getTypePtr()); + addType(TD->getLocation(),

[PATCH] D66516: [clangd] Highlight typedefs to template parameters as template parameters

ilya-biryukov updated this revision to Diff 219301. ilya-biryukov marked 5 inline comments as done. ilya-biryukov added a comment. - Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66516/new/ https://reviews.llvm.org/D66516 Files:

[PATCH] D67277: [clangd] Replace HighlightingKind::NumKinds with LastKind. NFC

ilya-biryukov added a comment. In D67277#1662669 , @hokein wrote: > LGTM, though I think the current implementation is a common pattern. Thanks. It's indeed a common pattern. The problem is that it forces clients to handle this extra case in every

[PATCH] D67341: [clangd] Simplify semantic highlighting visitor

ilya-biryukov created this revision. ilya-biryukov added a reviewer: hokein. Herald added subscribers: kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. - Functions to compute highlighting kinds for things are separated from the ones that add highlighting tokens. This keeps

[PATCH] D67290: [clangd] Add a new highlighting kind for typedefs

ilya-biryukov created this revision. ilya-biryukov added a reviewer: hokein. Herald added subscribers: kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. ilya-biryukov added a parent revision: D66516: [clangd] Highlight typedefs to template parameters as template parameters.

[PATCH] D67290: [clangd] Add a new highlighting kind for typedefs

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:530 + case HighlightingKind::Typedef: +return "entity.name.type.typedef.cpp"; case HighlightingKind::Namespace:

[PATCH] D67277: [clangd] Replace HighlightingKind::NumKinds with LastKind. NFC

ilya-biryukov created this revision. ilya-biryukov added a reviewer: hokein. Herald added subscribers: kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. The latter simplifies the client code by avoiding the need to handle it as a separate case statement. Repository: rG LLVM

[PATCH] D66516: [clangd] Highlight typedefs to template parameters as template parameters

ilya-biryukov added a comment. I'll send a separate change to fallback to a new highlighting type if the underlying type of a typedef is complicated. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66516/new/ https://reviews.llvm.org/D66516

[PATCH] D66516: [clangd] Added highlighting to types dependant on templates.

ilya-biryukov updated this revision to Diff 219100. ilya-biryukov added a comment. - Only highlight typedefs to non-composite types, simplifies everything Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66516/new/ https://reviews.llvm.org/D66516

[PATCH] D67274: [clangd] Improve output of semantic highlighting tests in case of failures

ilya-biryukov created this revision. ilya-biryukov added a reviewer: hokein. Herald added subscribers: kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. Instead of matching lists of highlightings, we annotate input code with resulting highlightings and diff it against the

[PATCH] D66516: [clangd] Added highlighting to types dependant on templates.

ilya-biryukov planned changes to this revision. ilya-biryukov added a comment. Now that I think about it more, maybe the simplest option is to not recurse into composite types at all E.g. a typedef to a template parameter will be highlighted as a template parameter. A typedef to a pointer

[PATCH] D66516: [clangd] Added highlighting to types dependant on templates.

ilya-biryukov updated this revision to Diff 219090. ilya-biryukov added a comment. - Replace UnderlyingTypeVisitor with a function - Remove PrintTo() - Also highlight references to typedefs - reformat Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. Not sure whether the new code is a win overall, it's still a bit messy Let me know what you think. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67172/new/ https://reviews.llvm.org/D67172

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov updated this revision to Diff 219061. ilya-biryukov added a comment. - Move system symbol mapping to a different class. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67172/new/ https://reviews.llvm.org/D67172 Files:

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. Ok, agreed. Adding this indirection layer is probably making things a bit too complex. I'll update the patch to clearly separate the std symbol mapping and canonical includes. In D67172#1660435 , @hokein wrote: > And I'm

[PATCH] D67224: [clangd] Enable completions with fixes in VSCode

ilya-biryukov planned changes to this revision. ilya-biryukov added a comment. Changing behavior of clangd might affect other clients. It would be nice to find alternative ways to fix this. Nevertheless, posting this here as an example of a potential workaround for the problem. Repository:

[PATCH] D67224: [clangd] Enable completions with fixes in VSCode

ilya-biryukov created this revision. Herald added subscribers: cfe-commits, kadircet, arphaman, mgrang, jkorous, MaskRay. Herald added a project: clang. Currently, the completions are not shown to the user as VSCode tries to match 'filterText' against the text in the edit range. Since the text

[PATCH] D67096: [clangd][vscode] Add a flag to enable semantic highlighting in clangd

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

[PATCH] D67096: [clangd][vscode] Add a flag to enable semantic highlighting in clangd

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts:113 const semanticHighlightingFeature = - new semanticHighlighting.SemanticHighlightingFeature(); + new semanticHighlighting.SemanticHighlightingFeature(

[PATCH] D67165: [clangd][vscode] Make SemanticHighlightingFeature more self-contained.

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

[PATCH] D67165: [clangd][vscode] Make SemanticHighlightingFeature more self-contained.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:88 +// recover from clangd crashes. +this.dispose(); +// Register the handler to handle semantic highlighting notification.

[PATCH] D67096: [clangd][vscode] Add a flag to enable semantic highlighting in clangd

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts:113 const semanticHighlightingFeature = - new semanticHighlighting.SemanticHighlightingFeature(); + new

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. Also considered moving the std symbol mapping into a separate (private-to-implementation) class, e.g. we don't use suffix mappings and symbol mappings outside system includes. But decided to wait until suffix mapping are going away completely. Repository: rG

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov created this revision. ilya-biryukov added reviewers: sammccall, hokein. Herald added subscribers: jfb, kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. This takes ~5% of time when running clangd unit tests. Repository: rG LLVM Github Monorepo

[PATCH] D66990: [clangd] Add distinct highlightings for declarations of functions and methods

ilya-biryukov added a comment. In D66990#1656230 , @nridge wrote: > Not a hard requirement, just a nice-to-have for someone moving from one tool > to another :) > If you feel that for now it's better not to do this, I can respect that. Thanks, if that

[PATCH] D67163: [Driver] Use shared singleton instance of DriverOptTable

ilya-biryukov created this revision. ilya-biryukov added reviewers: gribozavr, sammccall. Herald added a subscriber: kadircet. Herald added a project: clang. This significantly reduces the time required to run clangd tests, by ~10%. Should also have an effect on other tests that run command-line

[PATCH] D67117: [clangd] Split Preamble.h out of ClangdUnit.h. NFC

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

[PATCH] D67096: [clangd][vscode] Add a flag to enable semantic highlighting in clangd

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts:113 const semanticHighlightingFeature = - new semanticHighlighting.SemanticHighlightingFeature(); + new semanticHighlighting.SemanticHighlightingFeature(

[PATCH] D67096: [clangd][vscode] Add a flag to enable semantic highlighting in clangd

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts:113 const semanticHighlightingFeature = - new semanticHighlighting.SemanticHighlightingFeature(); + new semanticHighlighting.SemanticHighlightingFeature(

[PATCH] D67096: [clangd][vscode] Add a flag to enable semantic highlighting in clangd

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts:113 const semanticHighlightingFeature = - new semanticHighlighting.SemanticHighlightingFeature(); + new semanticHighlighting.SemanticHighlightingFeature(

[PATCH] D66990: [clangd] Add distinct highlightings for declarations of functions and methods

ilya-biryukov added a comment. In D66990#1655030 , @nridge wrote: > I was hoping though that a patch like this, which would bring us largely to > parity with Eclipse CDT's highlightings, wouldn't need to blocked on a change > to the upstream protocol

[PATCH] D66990: [clangd] Add distinct highlightings for declarations of functions and methods

ilya-biryukov added a comment. In D66990#1653833 , @nridge wrote: > I think it makes sense to style function declarations differently from > function uses for emphasis. For example, you can use the same color for both, > but make the declarations bold.

[PATCH] D66990: [clangd] Add distinct highlightings for declarations of functions and methods

ilya-biryukov added a comment. In D66990#1653833 , @nridge wrote: > There is precedent for highlighting declarations and uses differently in > other C++ editors. For example, Eclipse CDT has separate highlightings for > function and functions

[PATCH] D66995: [clangd] Add highlighting for macro expansions.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:35 +TraverseAST(AST.getASTContext()); +// Add highlightings for Macro Expansions as they are not traversed by the +// visitor. NIT: uncapitalize and

[PATCH] D66738: [clangd] Added highlighting for structured bindings.

ilya-biryukov added a comment. In D66738#1652285 , @hokein wrote: > I don't have a strong preference on how to highlight `BindingDecl`, but I > think highlighting them as `Variable` is better than no highlighting them at > all, so LGTM from myside.

[PATCH] D66516: [clangd] Added highlighting to types dependant on templates.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:177 return; +if (TP->isPointerType() || TP->isLValueReferenceType()) + // When highlighting dependant template types the type can be a pointer or

[PATCH] D66960: [Tooling] Migrated APIs that take ownership of objects to unique_ptr

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

[PATCH] D66928: [clangd] Collecting main file macro expansion locations in ParsedAST.

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/unittests/ClangdUnitTests.cpp:260 +// Macros from token

[PATCH] D66990: [clangd] Add distinct highlightings for declarations of functions and methods

ilya-biryukov added a comment. In D66990#1652172 , @jvikstrom wrote: > Should we have different highlightings for declarations vs usages? (Although > I guess in the end it will be up to the editor if they highlight them > differently as the scope is

[PATCH] D66947: Changed FrontendActionFactory::create to return a std::unique_ptr

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

[PATCH] D66928: [clangd] Collecting main file macro expansion locations in ParsedAST.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:107 +class CollectMainFileMacroExpansions : public PPCallbacks { + const SourceManager jvikstrom wrote: > ilya-biryukov wrote: > > Maybe make this part of

[PATCH] D66928: [clangd] Collecting main file macro expansion locations in ParsedAST.

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:107 +class CollectMainFileMacroExpansions : public PPCallbacks { + const SourceManager Maybe make this part of

[PATCH] D66751: [clangd] Add targetDecl(), which determines what declaration an AST node refers to.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. In D66751#1646991 , @sammccall wrote: > Yes. I actually ripped this out of the patch, because it wasn't related > enough and the patch is large. Makes sense and thanks for summing it

[PATCH] D66928: [clangd] Collecting main file macro expansion locations in ParsedAST.

ilya-biryukov added a comment. Looks great, just requests for comments and more test-cases from my side Comment at: clang-tools-extra/clangd/ClangdUnit.h:148 + /// All macro expansion locations in the main file. + std::vector MainFileMacroExpLocs; NIT:

[PATCH] D66877: Moved the IndexDataConsumer::finish call into the IndexASTConsumer from IndexAction

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 Comment at: clang/lib/Index/IndexingAction.cpp:77 +IndexCtx->getDataConsumer().setPreprocessor(PP); +

[PATCH] D66878: [Index] Stopped wrapping FrontendActions in libIndex and its users

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 Comment at: clang-tools-extra/clangd/index/IndexAction.cpp:217 std::unique_ptr Includes; +

[PATCH] D66878: [Index] Stopped wrapping FrontendActions in libIndex and its users

ilya-biryukov added a comment. Overall LG, thanks! Not sure why we need to keep `IndexingOptions` everywhere, though, see the relevant comment. Comment at: clang-tools-extra/clangd/index/IndexAction.cpp:217 std::unique_ptr Includes; + index::IndexingOptions Opts;

[PATCH] D66877: Moved the IndexDataConsumer::finish call into the IndexASTConsumer from IndexAction

ilya-biryukov added inline comments. Comment at: clang/lib/Index/IndexingAction.cpp:77 +IndexCtx->getDataConsumer().setPreprocessor(PP); +PP->addPPCallbacks(std::make_unique(IndexCtx)); + } The fact that we call `addPPCallbacks` **after** creating

[PATCH] D66876: Indexing: create PP callbacks in the ASTConsumer

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

[PATCH] D66875: [Index] Marked a bunch of classes 'final'

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

[PATCH] D66516: [clangd] Added highlighting to types dependant on templates.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:177 return; +if (TP->isPointerType() || TP->isLValueReferenceType()) + // When highlighting dependant template types the type can be a pointer or

[PATCH] D66759: [clangd] Surface errors from command-line parsing

ilya-biryukov updated this revision to Diff 217581. ilya-biryukov added a comment. - Remove accidental change Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66759/new/ https://reviews.llvm.org/D66759 Files:

[PATCH] D66759: [clangd] Surface errors from command-line parsing

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:465 +Diags.insert(Diags.end(), Preamble->Diags.begin(), Preamble->Diags.end()); + // Finally, add diagnostics coming from the AST. + { kadircet wrote: > ilya-biryukov

[PATCH] D66759: [clangd] Surface errors from command-line parsing

ilya-biryukov updated this revision to Diff 217579. ilya-biryukov marked 2 inline comments as done and an inline comment as not done. ilya-biryukov added a comment. - Do not flushDiag() on EndSourceFile - Do not reset WasAdjusted outside flusLastDiag() - Add a test that unknown warnings do not

[PATCH] D66759: [clangd] Surface errors from command-line parsing

ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. Just to clarify: we only want to surface errors, not warnings from command-line to avoid too much nosie; I'm totally on board with not spamming users with "unknown compiler warning" errors at the start of each file.

[PATCH] D66797: ArrayRef'ized CompilerInvocation::CreateFromArgs

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

[PATCH] D66731: [Driver] Add an option for createInvocationFromCommandLine to recover on errors

ilya-biryukov added inline comments. Comment at: clang/include/clang/Frontend/CompilerInvocation.h:150 /// + /// If an error was encountered while parsing the arguments, \returns false + /// and attempts to recover and continue parsing the rest of the arguments.

[PATCH] D66516: [clangd] Added highlighting to types dependant on templates.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:177 return; +if (TP->isPointerType() || TP->isLValueReferenceType()) + // When highlighting dependant template types the type can be a pointer or

[PATCH] D66731: [Driver] Add an option for createInvocationFromCommandLine to recover on errors

ilya-biryukov updated this revision to Diff 217335. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. - Start a paragrah with \returns instead of putting it in the middle Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D66759: [clangd] Surface errors from command-line parsing

ilya-biryukov added a comment. In D66759#1646509 , @kadircet wrote: > Thanks! This looks really useful when we can't build an AST due to unknown > compiler commands, but I am not sure about how useful it is to surface > command-line parsing errors once

[PATCH] D66751: [clangd] Add targetDecl(), which determines what declaration an AST node refers to.

ilya-biryukov added a comment. Mostly LGTM, thanks! Just a few clarifying questions and suggestions This API allows us to get a target declaration for a reference, but does not help with finding the source locations for those references. Do you plan to put this into the API? Have this as a

[PATCH] D66788: Refactor GlobList from an ad-hoc linked list to a vector

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/clang-tidy/GlobList.cpp:46 +GlobList::GlobList(StringRef Globs) { + do { +GlobListItem Item; NIT: I

[PATCH] D66787: GlobList: added a clear test for pattern priority

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

[PATCH] D66731: [Driver] Add an option for createInvocationFromCommandLine to recover on errors

ilya-biryukov updated this revision to Diff 217320. ilya-biryukov added a comment. - Fix english grammar in the comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66731/new/ https://reviews.llvm.org/D66731 Files:

[PATCH] D66731: [Driver] Add an option for createInvocationFromCommandLine to recover on errors

ilya-biryukov updated this revision to Diff 217319. ilya-biryukov added a comment. - Add a comment to CreateFromArgs Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66731/new/ https://reviews.llvm.org/D66731 Files:

[PATCH] D66731: [Driver] Add an option for createInvocationFromCommandLine to recover on errors

ilya-biryukov added a comment. In D66731#1644840 , @gribozavr wrote: > - In the doc comment for `CompilerInvocation::CreateFromArgs`, could you add > a note that it does a best effort to provide a CompilerInvocation even if it > returns false? Right

[PATCH] D66759: [clangd] Surface errors from command-line parsing

ilya-biryukov created this revision. ilya-biryukov added a reviewer: kadircet. Herald added subscribers: arphaman, jkorous, MaskRay, javed.absar. Herald added a project: clang. ilya-biryukov added a parent revision: D66731: [Driver] Add an option for createInvocationFromCommandLine to recover on

[PATCH] D66747: Moved GlobList into a separate header file

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

[PATCH] D66731: [Driver] Add an option for createInvocationFromCommandLine to recover on errors

ilya-biryukov created this revision. ilya-biryukov added a reviewer: gribozavr. Herald added subscribers: kadircet, arphaman, jkorous. Herald added a project: clang. Previously, it would always return nullptr on any error. This change adds a parameter, controlling whether the function should

[PATCH] D66723: [clangd] Add a distinct highlighting for local variables

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:38 Primitive, + LocalVariable, NIT: maybe put it right after `Variable`? IIUC, we do not rely on actual numeric values being the same across different clangd

[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:644 + assert(It.second.Edits && "TextEdits hasn't been generated?"); + if (auto Draft = DraftMgr.getDraft(It.first()))

[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:644 + assert(It.second.Edits && "TextEdits hasn't been generated?"); + if (auto Draft = DraftMgr.getDraft(It.first())) { +llvm::StringRef Contents =

[PATCH] D66516: [clangd] Added highlighting to types dependant on templates.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:177 return; +if (TP->isPointerType() || TP->isLValueReferenceType()) + // When highlighting dependant template types the type can be a pointer or

[PATCH] D66592: [clangd] Send suppported codeActionKinds to the client.

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/ClangdLSPServer.cpp:467 + // Per LSP, codeActionProvide can be either boolean or CodeActionOptions. + //

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