[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

ilya-biryukov added a comment. Keeping it **only** in the cpp file also LG if we don't plan to have other usages for now, but please remove the version from `FixIt.h` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59376/new/ https://reviews.llvm.or

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

ilya-biryukov added a comment. Given that we don't any usages of `getExtendedRange`, could you remove it from the `Fixit.h` and move to a header somewhere in the `clangToolingRefactoring` (e.g. into `Transformer.h` itself)? Having two copies of this functions in the codebase does not look good,

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

ilya-biryukov added a comment. Sam, thanks for taking a look and the useful comments! @ioeric, I second Sam's suggestion to split the compile command and the fallback action into two changes. This would make it easier to review those in isolation. Could you do this please? C

[PATCH] D60194: [Tooling] add a Heuristic field indicating that a CompileCommand was guessed.

ilya-biryukov added a comment. I'll leave the LGTM to Manuel. I don't have a strong opinion here, but here are some thoughts. The approach taken seems to be the least-leaky that I can think of unless we want to change the return type of `CompilationDatabase::getCompileCommand` or add a new met

[PATCH] D60258: [CodeComplete] Fix crash when completing ObjC block parameter with a broken type

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60258/new/ https://reviews.llvm.org/D60258 ___

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

ilya-biryukov added a comment. In D59376#1454748 , @ymandel wrote: > I propose that I create a new library in Core with `getExtendedRange()` and > remove it from FixIt. The other utility functions that I need will also go > there. We can separately inv

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

ilya-biryukov added a subscriber: ioeric. ilya-biryukov added a comment. Per @ioeric's suggestion: why not move the helper into `Tooling/Refactoring/ExtendedRange.h`? If it's in `ToolingRefactoring`, both stencil and transformer can access it. For external users, a dependency on either `ToolingC

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. One more LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59376/new/ https://reviews.llvm.org/D59376 ___ cfe-commits mailing list

[PATCH] D60269: [LibTooling] Add "SourceCode" library for functions relating to source-code manipulation.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. Nice! A more specific name and a more specific library. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60269/new/ https://rev

[PATCH] D60194: [Tooling] add a Heuristic field indicating that a CompileCommand was guessed.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM, Manuel does not mind having this and the string heuristic does look like the easiest approach to convey this information in the API. Repository: rC Clang CHANGES SINCE

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

ilya-biryukov updated this revision to Diff 193901. ilya-biryukov added a comment. Pass an enum indicating where the token comes from. The enum is ad-hoc at the moment, will require some thought to turn it into a reasonable abstraction. The consumer of the token stream actually needs to be able t

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 193903. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. Herald added a subscriber: mgrang. Changes: - Add multi-file support, record a single expanded stream and per-file-id raw token streams and mappings. - Rename MacroI

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

ilya-biryukov added inline comments. Comment at: clangd/ClangdUnit.cpp:405 - std::vector Diags = ASTDiags.take(); + auto ShouldSurfaceDiag = [](const Diag &D) { +if (D.Severity == DiagnosticsEngine::Level::Error || The name suggests we filter **all** diag

[PATCH] D59605: [clangd] Introduce background-indexer

ilya-biryukov added a comment. I would propose to change the name of the tool. The "background" part only makes sense in the context of running inside clangd, the index stored on disk is not "background" in any manner. I'd go with something like `build-clangd-index` (the best option is probably

[PATCH] D60267: [clangd] Support relatedInformation in diagnostics.

ilya-biryukov added inline comments. Comment at: clangd/Diagnostics.cpp:185 OS << Note.Message; - OS << "\n\n"; - printDiag(OS, Main); + // If there's no structured link between the note and the original diagnostic + // then emit the main diagnostic to give context. --

[PATCH] D60267: [clangd] Support relatedInformation in diagnostics.

ilya-biryukov added inline comments. Comment at: clangd/Diagnostics.cpp:271 + if (!Note.AbsFile) { +log("Dropping note from unknown file: {0}", Note); +continue; Maybe `vlog`? This is what we use for dropped diagnostics, should probably stic

[PATCH] D60267: [clangd] Support relatedInformation in diagnostics.

ilya-biryukov added inline comments. Comment at: clangd/Protocol.cpp:280 R.DiagnosticFixes = *CodeActions; + if (auto CodeActions = Diagnostics->getBoolean("relatedInformation")) +R.DiagnosticRelatedInformation = *CodeActions; kadircet wrote

[PATCH] D60510: [ADT] Fix template parameter names of llvm::{upper|lower}_bound

ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added subscribers: llvm-commits, dexonsmith. Herald added a project: LLVM. Rename template parameter for a search value from 'ForwardIt' to 'T'. While here, also use perfect forwarding to pass the value to STL a

[PATCH] D59641: [clangd] Show template argument list in workspacesymbols and documentsymbols responses

ilya-biryukov added a comment. LG, just have a simple clarifying question. Comment at: clang-tools-extra/clangd/AST.cpp:112 if (!Out.str().empty()) { -// FIXME(ibiryukov): do not show args not explicitly written by the user. -if (auto *ArgList = getTemplateSpecializa

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 194680. ilya-biryukov marked 19 inline comments as done. ilya-biryukov added a comment. - Remove a spamy debug tracing output. - Less debug output, move stuff around, more comments. - Add methods that map expanded tokens to raw tokens. - Rename toOffsets

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 194682. ilya-biryukov added a comment. - Fix header comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59887/new/ https://reviews.llvm.org/D59887 Files: clang/include/clang/Tooling/Syntax/Tokens.h

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov added a comment. The new version address most of the comments, there are just a few left in the code doing the mapping from Dmitri, I'll look into simplifying and removing the possibly redundant checks. Please take a look at this version, this is very close to the final state. =

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 194683. ilya-biryukov added a comment. - Store a source manager in a token buffer Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59887/new/ https://reviews.llvm.org/D59887 Files: clang/include/clang/Too

[PATCH] D59605: [clangd] Introduce background-indexer

ilya-biryukov added a subscriber: sammccall. ilya-biryukov added a comment. To reiterate the offline discussion: the tool seems useful to me, but it would be best to keep the client code simpler, it's currently fighting with `BackgroundIndex` because it's trying to hijack some of its functionali

[PATCH] D59605: [clangd] Introduce background-indexer

ilya-biryukov added a comment. To make it clear, I think the question is not just "which part of functionality is missing in BackgroundIndex", it's rather "which part of BackgroundIndex we **don't** need". Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D59641: [clangd] Show template argument list in workspacesymbols and documentsymbols responses

ilya-biryukov accepted this revision. ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/AST.cpp:112 if (!Out.str().empty()) { -// FIXME(ibiryukov): do n

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:566 +printTemplateSpecializationArgs(ND); +S.TemplateSpecializationArgs = TemplateSpecializationArgs; if (Opts.StoreAllDocumentation) Any reason t

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:566 +printTemplateSpecializationArgs(ND); +S.TemplateSpecializationArgs = TemplateSpecializationArgs; if (Opts.StoreAllDocumentation) kadircet wro

[PATCH] D60408: [LibTooling] Extend Transformer to support multiple simultaneous changes.

ilya-biryukov added a comment. Sorry for the delay. There seem to be a few changes that are unrelated to the actual patch. Could we separate various non-functional changes (moving code around, etc.) into a separate change to keep the diff for this one minimal? Comment at: cla

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

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

[PATCH] D60560: [clangd] Enable clang-tidy by default.

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

[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

ilya-biryukov added a comment. This is long overdue Comment at: clangd/TUScheduler.cpp:338 + Barrier(Barrier), Done(false) { + FileInputs.CompileCommand = CDB.getFallbackCommand(FileName); +} The command is filled with a fallback by `ClangdServer`, right?

[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

ilya-biryukov added inline comments. Comment at: clangd/TUScheduler.cpp:338 + Barrier(Barrier), Done(false) { + FileInputs.CompileCommand = CDB.getFallbackCommand(FileName); +} ioeric wrote: > ilya-biryukov wrote: > > The command is filled with a fallback b

[PATCH] D60408: [LibTooling] Extend Transformer to support multiple simultaneous changes.

ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:80 +// \endcode +struct TextChange { + // The (bound) id of the node whose source will be replaced. This id should ymandel wrote: > ilya-biryukov wrote: > > `

[PATCH] D60408: [LibTooling] Extend Transformer to support multiple simultaneous changes.

ilya-biryukov added a comment. In D60408#1464364 , @ymandel wrote: > I've done that as far as I can tell. Please let me know if I've missed > anything. Many thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

ilya-biryukov added inline comments. Comment at: clangd/TUScheduler.cpp:552 +const tooling::CompileCommand &ASTWorker::getCurrentCompileCommand() const { + return FileInputs.CompileCommand; +} ioeric wrote: > ilya-biryukov wrote: > > ioeric wrote: > > > ilya-bir

[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

ilya-biryukov added inline comments. Comment at: clangd/TUScheduler.cpp:552 +const tooling::CompileCommand &ASTWorker::getCurrentCompileCommand() const { + return FileInputs.CompileCommand; +} ioeric wrote: > ilya-biryukov wrote: > > ioeric wrote: > > > ilya-bir

[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

ilya-biryukov added a comment. Thanks, the change LG now. Only nits from my side! Comment at: clangd/Compiler.h:19 #include "../clang-tidy/ClangTidyOptions.h" +#include "GlobalCompilationDatabase.h" #include "index/Index.h" NIT: this looks unrelated to the ac

[PATCH] D60408: [LibTooling] Extend Transformer to support multiple simultaneous changes.

ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:80 +// \endcode +struct TextChange { + // The (bound) id of the node whose source will be replaced. This id should ymandel wrote: > ilya-biryukov wrote: > > y

[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM! See the NITs, specifically about `runAddDocument` - those definitely look worth fixing. Comment at: unittests/clangd/ClangdTests.cpp:1148 + EXPECT_EQ(Re

[PATCH] D60408: [LibTooling] Extend Transformer to support multiple simultaneous changes.

ilya-biryukov added a comment. A few quick responses, will take a closer look again tomorrow. Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:87 + TextGenerator Replacement; + TextGenerator Explanation; +}; ymandel wrote: > ilya-biryukov wro

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

ilya-biryukov added a comment. Sorry, lost this revision. Looking now Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59302/new/ https://reviews.llvm.org/D59302 ___ cfe-commits mailing list cfe-commits@li

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

ilya-biryukov added inline comments. Comment at: clangd/ClangdUnit.cpp:251 + auto Res = + std::lower_bound(MainFileIncludes.begin(), MainFileIncludes.end(), Inc, + [](const Inclusion &LHS, const Inclusion &RHS) { NIT: use `llvm::lower_

[PATCH] D60408: [LibTooling] Extend Transformer to support multiple simultaneous changes.

ilya-biryukov added a comment. Thanks for the update, looks good! Just a few nits from my side. > The only part I don't love (here and elsewhere) is the duplicate overloads > one each for std::string and TextGenerator. Totally agree, that's not ideal. Would be nice to find a way to workaround t

[PATCH] D60408: [LibTooling] Extend Transformer to support multiple simultaneous changes.

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

[PATCH] D60408: [LibTooling] Extend Transformer to support multiple simultaneous changes.

ilya-biryukov added a comment. Not sure if you want to land this before or after stencil. This seems useful even without the latter. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60408/new/ https://reviews.llvm.org/D60408 __

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

ilya-biryukov added inline comments. Comment at: clang/include/clang/Lex/Preprocessor.h:120 +/// callback that records tokens. +enum class TokenSource { + File, // a token coming directly from a file that is not a macro directive, This is supposed to provide eno

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 195425. ilya-biryukov added a comment. - Simplify rawByExpanded by using a helper function. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59887/new/ https://reviews.llvm.org/D59887 Files: clang/include

[PATCH] D59605: [clangd] Introduce background-indexer

ilya-biryukov added a comment. Leaving a summary of the offline discussion here. @sammccall has pointed out maintaining and shipping more tools is work and we should probably avoid adding them upstream without careful design. We ended up deciding to **not** land this particular upstream just yet

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

ilya-biryukov added a comment. Found an issue today. Input (break a line at ^): // here is my comment ^// another comment Expected: // here is my comment ^// another comment Actual (an extra comment marker is added): // here is my comment // // another comment Repository: rCTE

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 196462. ilya-biryukov marked 53 inline comments as done. ilya-biryukov added a comment. - Simplify rawByExpanded by using a helper function. - Add a FIXME to add spelled-to-expanded mapping - s/raw*/spelled* - Split token collector and token buffer tests

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:59 + explicit Token(const clang::Token &T); + + tok::TokenKind kind() const { return Kind; } sammccall wrote: > Token should be copyable I think? Sure, they are copyabl

[PATCH] D59814: [Testing] Move clangd::Annotations to llvm testing support

ilya-biryukov updated this revision to Diff 196592. ilya-biryukov added a comment. - Add simple tests - Add equality and stream output operators for Range Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59814/new/ https://reviews.llvm.org/D59814 Fil

[PATCH] D59814: [Testing] Move clangd::Annotations to llvm testing support

ilya-biryukov updated this revision to Diff 196599. ilya-biryukov added a comment. - Added a death test for error conditions Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59814/new/ https://reviews.llvm.org/D59814 Files: clang-tools-extra/unitte

[PATCH] D61120: [clangd] Optimize "don't include me" check.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM (see the comment about a typo, though) Comment at: clangd/index/SymbolCollector.cpp:669 +return false; + return Line.contains_lower("includ"); +}

[PATCH] D61122: [clangd] Don't build clangd or run its tests when LLVM_ENABLE_THREADS is off, unless specifically directed to do so

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. A better name could definitely be nice, e.g. `BUILD_CLANGD` would've been much nicer. But the current one is also okayish, being consistent with existing names is a good reason t

[PATCH] D61120: [clangd] Optimize "don't include me" check.

ilya-biryukov added inline comments. Comment at: clangd/index/SymbolCollector.cpp:669 +return false; + return Line.contains_lower("includ"); +} sammccall wrote: > ilya-biryukov wrote: > > A typo? Should it be `include`. > it's meant to match "include" or "in

[PATCH] D61015: [LibTooing] Change Transformer's TextGenerator to a partial function.

ilya-biryukov added a comment. Why would we consider this a legitimate failure, rather than a programming error? Same argument could be made about any form of format-string-like functions, e.g. `llvm::formatv` or `sprintf`. Yet, they return strings and not `Expected` or their equivalent. Repos

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/Background.h:113 + // Note that FileSymbols counts References by incrementing it per each file + // mentioning the symbol, including headers. This contradicts with the We should a

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

ilya-biryukov added inline comments. Comment at: clangd/Diagnostics.cpp:78 +// offsets when displaying that information to users. +Position toOneBased(Position P) { + ++P.line; Could we avoid introducing a function that breaks the invariant of a type? Having a f

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

ilya-biryukov added a comment. Another example: int test() { ^ } Expected: a newline was added. Actual: newline does not allow to be added. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60605/new/ https://reviews.llvm.org/D60605

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

ilya-biryukov added a comment. Input: int test() { }^ Expected: int test() { } ^ Actual: int test() {} ^ Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60605/new/ https://reviews.llvm.org/D60605 _

[PATCH] D61015: [LibTooing] Change Transformer's TextGenerator to a partial function.

ilya-biryukov added a comment. I'd argue it's the server's job to validate the inputs in that case. The code that landed so far clearly looks like a C++ DSL to describe transformations of the source. While it **can** be used a dependency in the server-side, I don't see why doing user-input chec

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

ilya-biryukov added a comment. In D60605#1478922 , @sammccall wrote: > In D60605#1478581 , @ilya-biryukov > wrote: > > > Input: > > > > int test() { > > }^ > > > > > > Actual: > > > > int test() {} > > ^ >

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

ilya-biryukov added a comment. Input: // this is my comment. ^ // and it continues on the next line. Expected: // this is my comment. // ^ // and it continues on the next line. Actual: // this is my comment. ^ // and it continues on the next line. Repository: rCTE Clang To

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

ilya-biryukov added a comment. Input: if (something) foo; // a comment^ Expected: if (something) foo; // a comment ^ Actual (indented to align with a comment): if (something) foo; // a comment ^ Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION h

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 196847. ilya-biryukov added a comment. - s/llvm::unittest::ValueIs/llvm::ValueIs. - Add tests for empty macro expansions and directives around macro expansions. - Record all gaps between spelled and expanded tokens. - Tweak test dump to distinguish diffe

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov added a comment. @sammccall, could we do another round of review? I think it's perfect now... (Just kidding :-) ) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59887/new/ https://reviews.llvm.org/D59887

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 196849. ilya-biryukov added a comment. - Update a comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59887/new/ https://reviews.llvm.org/D59887 Files: clang/include/clang/Tooling/Syntax/Tokens.h c

[PATCH] D61187: [clangd] Move clangd tests to clangd directory. check-clangd is no longer part of check-clang-tools.

ilya-biryukov added a comment. For the record: the tests pass in the shared build configuration. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61187/new/ https://reviews.llvm.org/D61187 ___ cfe-commits

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

ilya-biryukov added a comment. This looks nice and minimal, thanks! Happy to LGTM this, unless @sammccall want to take another look (e.g. to account for related information patch) Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59302/new/ https://rev

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

ilya-biryukov added inline comments. Comment at: clangd/Diagnostics.cpp:115 + const SourceManager &SM = Info.getSourceManager(); + std::vector IncludeStack; + auto GetIncludeLoc = [&SM](SourceLocation SLoc) { replace `vector` with `SourceLocation` now that we

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/Background.h:113 + // Note that FileSymbols counts References by incrementing it per each file + // mentioning the symbol, including headers. This contradicts with the kadircet wr

[PATCH] D61015: [LibTooing] Change Transformer's TextGenerator to a partial function.

ilya-biryukov added a comment. In D61015#1478886 , @ymandel wrote: > The problem is that validation can't* be done in the abstract. It has to be > done with respect to a specific match result. Unfortunately, the server won't > be layered directly on top

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

ilya-biryukov added a comment. Found today: if (something) return; // some long comment // that takes two lines.^ Expected: comment on last line is not re-indented. Actual (comment re-indented): if (something) return; // some long comment // that takes two lines. Rep

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

ilya-biryukov updated this revision to Diff 197278. ilya-biryukov added a comment. - Revamp TokenSource, make it more principled Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59885/new/ https://reviews.llvm.org/D59885 Files: clang/include/clang/

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 197279. ilya-biryukov added a comment. - Simplify collection of tokens - Move dumping code to the bottom of the file Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59887/new/ https://reviews.llvm.org/D5988

[PATCH] D61015: [LibTooling] Change Transformer's TextGenerator to a partial function.

ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:47 -using TextGenerator = -std::function; +// \c TextGenerator may fail, because it processes dynamically-bound match +// results. For example, a typo in the name of a bo

[PATCH] D61015: [LibTooling] Change Transformer's TextGenerator to a partial function.

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

[PATCH] D60937: [clangd] Fix code completion of macros defined in the preamble region of the main file.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. I suggest considering a more direct way to solve the same problem: we could collect symbols for the main-file macros when building the preamble and store them in `Preamble

[PATCH] D60937: [clangd] Fix code completion of macros defined in the preamble region of the main file.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. LGTM. Neat! Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60937/new/ https://reviews.llvm.org/D60937 ___ cfe-commits mailing list cfe

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. In D59885#1485159 , @rsmith wrote: > I'd like to understand more about the intended use cases of this > functionality. What information do clients want? We are aiming to coll

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang/lib/Lex/Preprocessor.cpp:956-957 --LexLevel; + if (OnToken) +OnToken(Result, Source); } rsmith wrote: > This seems like it's going to be extremely hard

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

ilya-biryukov updated this revision to Diff 197778. ilya-biryukov added a comment. - Simplify the interface, get only the expanded token stream - An attempt to not report tokens from delayed parsing twice, almost works Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://re

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

ilya-biryukov marked an inline comment as not done. ilya-biryukov added inline comments. Comment at: clang/lib/Lex/Preprocessor.cpp:956-957 --LexLevel; + if (OnToken) +OnToken(Result, Source); } ilya-biryukov wrote: > rsmith wrote: > > This seems like it

[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

ilya-biryukov added a comment. In D60605#1485375 , @sammccall wrote: > I'm not able to reproduce this, can you give a complete example? After adding a newline here: int test(bool x) { if (x) return 10; // All spelled tokens are accounted for

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 197803. ilya-biryukov added a comment. - Get only expanded stream from the preprocessor, recompute mappings and spelled stream separately Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59887/new/ https://

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

ilya-biryukov marked an inline comment as not done. ilya-biryukov added inline comments. Comment at: clang/lib/Lex/Preprocessor.cpp:956-957 --LexLevel; + if (OnToken) +OnToken(Result, Source); } ilya-biryukov wrote: > ilya-biryukov wrote: > > rsmith wrot

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

ilya-biryukov added a comment. - Could you provide a few real-world motivating examples? Especially interested in cases that were complicated before and are easy to do now? - Do we expect most the rules to be written using the new API or is this something that's gonna be used in `5-10%` of the

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

ilya-biryukov updated this revision to Diff 197986. ilya-biryukov marked 7 inline comments as done. ilya-biryukov added a comment. - Rebase - Parse client capabilities, send markdown on hover if client supports it - Use inline block for the scope - Add a language for code blocks - Add a FIXME for

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/FormattedString.cpp:41 + +void FormattedString::appendCodeBlock(std::string Code) { + Chunk C; sammccall wrote: > you may want to take the l

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.h:186 void findHover(PathRef File, Position Pos, - Callback> CB); + Callback> CB); kadircet

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

ilya-biryukov updated this revision to Diff 197989. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Return 'HoverInfo' instead of FormattedString - Reformat Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58547/new/ h

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

ilya-biryukov updated this revision to Diff 197991. ilya-biryukov added a comment. - Remove a FIXME that was fixed Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58547/new/ https://reviews.llvm.org/D58547 Files: clang-tools-extra/clangd/CMakeList

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

ilya-biryukov marked 2 inline comments as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/FormattedString.h:27 + /// Append plain text to the end of the string. + void appendText(std::string Text); + /// Append a block of C++ code. This translat

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.h:186 void findHover(PathRef File, Position Pos, - Callback> CB); + Callback> CB); ilya-biryukov wrote: > kadircet wrote: > > sammccall wro

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

ilya-biryukov added a comment. This is ready for another round of review. As mentioned in the comments, I think some of the comments are better addressed by a separate change, because they would touch lots of lines in the test. Let me know if I missed anything else. Repository: rG LLVM Githu

[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

ilya-biryukov updated this revision to Diff 197994. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Remove dead test code. NFC Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58547/new/ https://reviews.llvm.org/D58547

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

ilya-biryukov added a comment. > As for naming, agreed, but does that concern drop away once we have only a > single RewriteRule definition? Sure, that won't be an issue. The use-cases make sense, thanks for the examples. Could you add one or two to the code? Would probably make sense to expre

[PATCH] D61495: [clangd] Use more relaxed fuzzy-matching rules for local variables and class members.

ilya-biryukov added a comment. With two modes of operation, could inconsistencies between the client-side and the server-side filtering become more/less of an issue? Is it true that the clients (VSCode, etc) do something closer to `Lax` mode? Repository: rCTE Clang Tools Extra CHANGES SINCE

<    8   9   10   11   12   13   14   15   16   17   >