[PATCH] D53866: [Preamble] Stop generating preamble for circular #includes

ilya-biryukov added inline comments. Comment at: include/clang/Lex/Preprocessor.h:391 } PreambleConditionalStack; + bool PreambleGenerationFailed = false; nik wrote: > ilya-biryukov wrote: > > There's a mechanism to handle preamble with errors, see > > `Pr

[PATCH] D55315: [clangd] Only reduce priority of a thread for indexing.

ilya-biryukov added inline comments. Comment at: clangd/Threading.h:124 }; -void setThreadPriority(std::thread &T, ThreadPriority Priority); +// Sets scheduling priority for the calling thread. +void setCurrentThreadPriority(ThreadPriority Priority); The comment

[PATCH] D55085: Avoid emitting redundant or unusable directories in DIFile metadata entries

ilya-biryukov added a comment. Ah, sorry, I incorrectly checked that the old revision landed without problems. Have no data on whether the new revision breaks anything, will have to wait for the input from the US timezone buildcop CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55085/new

[PATCH] D54945: This commit adds a chapter about clang-tidy integrations

ilya-biryukov added a comment. Maybe try to use Arcanist for uploading diffs? The web interface can be confusing at times, Arcanist is more reliable (LLVM docs have pointers to the Arcanist documentation to get st

[PATCH] D55417: [clangd] Change diskbackedstorage to be atomic

ilya-biryukov added a comment. NIT: I believe we're missing a hyphen in the change description, i.e. it should be `disk-backed`. Comment at: clangd/index/BackgroundIndexStorage.cpp:76 +// Write to a temporary file first. +llvm::SmallString<64> TempPath; +int FD; --

[PATCH] D55315: [clangd] Only reduce priority of a thread for indexing.

ilya-biryukov added inline comments. Comment at: clangd/index/Background.cpp:202 std::lock_guard Lock(QueueMu); -Queue.push_back(std::move(T)); +if (Priority == ThreadPriority::Low) { + Queue.push_back(Bind( kadircet wrote: > ilya-biryukov wrote

[PATCH] D53866: [Preamble] Stop generating preamble for circular #includes

ilya-biryukov added inline comments. Comment at: include/clang/Basic/DiagnosticLexKinds.td:432 +def err_pp_including_mainfile_for_preamble : Error< + "main file cannot be included recursively for preamble">, DefaultFatal; def err_pp_empty_filename : Error<"empty filename">; ---

[PATCH] D55359: [clangd] Avoid emitting Queued status when we are able to acquire the Barrier.

ilya-biryukov added inline comments. Comment at: clangd/TUScheduler.cpp:644 + std::unique_lock Lock(Barrier, std::try_to_lock); + if (Lock.owns_lock()) { +ExecuteAction(); Maybe simplify the code a bit? ``` // Replacing these two lines: // emit

[PATCH] D55431: [CodeComplete] Set preferred type to bool on conditions

ilya-biryukov created this revision. ilya-biryukov added a reviewer: kadircet. Repository: rC Clang https://reviews.llvm.org/D55431 Files: lib/Sema/CodeCompleteConsumer.cpp lib/Sema/SemaCodeComplete.cpp test/CodeCompletion/preferred-type.cpp Index: test/CodeCompletion/preferred-type.cp

[PATCH] D58340: [clang][Index] Visit UsingDecls and generate USRs for them

ilya-biryukov added inline comments. Comment at: include/clang/Index/IndexSymbol.h:75 UsingValue, + UsingDeclaration, }; We don't seem to use the subkind anywhere. Let's remove it. Comment at: unittests/Index/IndexTests.cpp:253 + tooling:

[PATCH] D58683: [clangd] Set thread priority on Windows

ilya-biryukov created this revision. ilya-biryukov added reviewers: kadircet, gribozavr. Herald added subscribers: arphaman, jkorous, MaskRay, ioeric. Herald added a project: clang. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D58683 Files: clang-tools-extra/clangd/Threading.

[PATCH] D58600: [clangd] Emit source to Diagnostic.

ilya-biryukov added inline comments. Comment at: clangd/ClangdUnit.cpp:380 if (Preamble) Diags.insert(Diags.begin(), Preamble->Diags.begin(), Preamble->Diags.end()); return ParsedAST(std::move(Preamble), std::move(Clang), std::move(Action), Preamble d

[PATCH] D58710: Added more detailed documentation for clangd

ilya-biryukov added a reviewer: ioeric. ilya-biryukov added a comment. Still looking at it, a few quick notes. Also added ioeric for an extra pair of eyes (it's a large change!). Comment at: clang-tools-extra/docs/clangd/Installation.rst:360 + +- Pass an experimental `-backgrou

[PATCH] D58710: Added more detailed documentation for clangd

ilya-biryukov added inline comments. Comment at: clang-tools-extra/docs/clangd/Features.rst:41 + +**(New in v9)** +If a missing symbol was seen in a file you've edited recently, clangd will It feels wrong to have announcements of the `9.0` features in the `8.0` r

[PATCH] D58710: Added more detailed documentation for clangd

ilya-biryukov added a comment. It feels we have two groups of docs here, do you think it would be useful to split them more explicitly? Specifically, we have: 1. User documentation - Getting started with clangd - Features 2. Developer documentation - Protocol extensions - Compiling clang

[PATCH] D58710: Added more detailed documentation for clangd

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

[PATCH] D58717: Added documentation for clangd v9+ features

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

[PATCH] D58721: Added release notes for clangd 8

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/docs/ReleaseNotes.rst:76 + +- clangd has a new LSP extension that allows the client to request code actions + to be sent tog

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

ilya-biryukov added a comment. To split this into multiple independent changes, we could start with storing the symbols for template specializations, but only showing the primary template in the results of workspaceSymbols. This would enable other features (xrefs, etc), and we could re-add spec

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

ilya-biryukov added a comment. A few small NITs Comment at: clang-tools-extra/clangd/ClangdServer.h:188 + /// Get information about type hierarchy for a given position. + void findTypeHierarchy(PathRef File, Position Pos, int Resolve, + TypeHierarchyDi

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

ilya-biryukov created this revision. ilya-biryukov added a reviewer: ioeric. Herald added subscribers: kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. It now gives less preference to prefix matches and does not penalize skipping segments. The motivation is producing better sc

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

ilya-biryukov added a comment. I'll collect the metrics and post them here too Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59300/new/ https://reviews.llvm.org/D59300 ___ cfe-commits mailing list cfe-

[PATCH] D58556: [LibTooling] Add "smart" retrieval of AST-node source to FixIt library

ilya-biryukov added a comment. Ah, had some comments and forgot to send them out before I went on vacation :-( Comment at: clang/include/clang/Tooling/FixIt.h:60 +// future to include more associated text (like comments). +CharSourceRange getSourceRangeAuto(const Stmt &S, ASTCo

[PATCH] D58556: [LibTooling] Add "smart" retrieval of AST-node source to FixIt library

ilya-biryukov added a comment. Ignore my previous comment, I'll take another look Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58556/new/ https://reviews.llvm.org/D58556 ___ cfe-commits mailing list c

[PATCH] D58556: [LibTooling] Add "smart" retrieval of AST-node source to FixIt library

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. `getExtendedRange` is a really good choice! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58556/new/ https://reviews.llvm.or

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

ilya-biryukov added a comment. Here are some initial measurements with the current metrics that we have. We seem to regress significantly in some cases. This is expected, since we only check the prefix matches there. @ioeric is working on including the non-prefix (more concretely, initialism ma

[PATCH] D59354: [clangd] Print arguments in template specializations

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/AST.cpp:57 +return Func->getTemplateSpecializationArgsAsWritten()->arguments(); + if (auto *Cls = llvm::dyn_cast(&ND)) +return Cls->getTemplateArgsAsWritten()->arguments(); For `Cl

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

ilya-biryukov updated this revision to Diff 190604. ilya-biryukov added a comment. - Adjust the tweaks, bring back removed heuristics. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59300/new/ https://reviews.llvm.org/D59300 Files: clang-tools-ex

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

ilya-biryukov added a comment. Here are the stats for the latest version. We now get a significant bump for initialisms and a much lower hit in non-initialism cases. ==

[PATCH] D59354: [clangd] Print arguments in template specializations

ilya-biryukov added a comment. Mostly nits, but could you elaborate why can't we print the type-loc when printing the template argument (see the corresponding comment)? Comment at: clang-tools-extra/clangd/AST.cpp:66 +return Var->getTemplateArgsInfo().arguments(); + retur

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

ilya-biryukov marked 5 inline comments as done. ilya-biryukov 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. +stati

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

ilya-biryukov updated this revision to Diff 190796. ilya-biryukov added a comment. - Added comments - Add a test case for 'onmes' Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59300/new/ https://reviews.llvm.org/D59300 Files: clang-tools-extra/c

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/XRefs.h:62 +/// Find the record type references at \p Pos. +const CXXRecordDecl *findRecordTypeAt(ParsedAST &AST, Position Pos); + nridge wrote: > ilya-biryukov wrote: > > This method looks

[PATCH] D59354: [clangd] Print arguments in template specializations

ilya-biryukov added a comment. The only important comment is about test coverage Comment at: clang-tools-extra/clangd/AST.cpp:84 +if (auto STL = TL.getAs()) { + std::vector ArgLocs; + for (unsigned I = 0; I < STL.getNumArgs(); I++) kadircet wrote:

[PATCH] D57532: [Index] Make sure c-index-test finds libc++ on Mac

ilya-biryukov added a comment. Lost this one after vacation, sorry. Still need to figure out why the test is failing. Any help appreaciated Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57532/new/ https://reviews.llvm.org/D57532

[PATCH] D59354: [clangd] Print arguments in template specializations

ilya-biryukov added inline comments. Comment at: clang/lib/AST/TypePrinter.cpp:1644 +llvm::raw_ostream &OS) { + A.getTypeSourceInfo()->getType().print(OS, PP); +} kadircet wrote: > ilya-biryukov wrote: > > kadircet wrote: > > > il

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

ilya-biryukov updated this revision to Diff 190803. ilya-biryukov marked 3 inline comments as done. ilya-biryukov added a comment. Address comments: - Shorten the comment to fit it into a single line. - Added a comment about single-case patterns Repository: rG LLVM Github Monorepo CHANGES SI

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

ilya-biryukov 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; ioeric wrote: > ilya-biryukov wrote: > > ioeric wrote: > >

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

ilya-biryukov updated this revision to Diff 190804. ilya-biryukov added a comment. - A better name for the new test case Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59300/new/ https://reviews.llvm.org/D59300 Files: clang-tools-extra/clangd/Fuz

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

ilya-biryukov added a comment. I'll look more closely into the details, but just a high-level question now: why would we want to make this optional and not simply surface these extra diagnostics? Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59302/

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

ilya-biryukov added a comment. Looks very polished, thanks! Will have to sink the change in a bit, will come back with more comments on Monday. In the meantime, a few initial comments and suggestions. Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:54 +/// bo

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

ilya-biryukov added a comment. Hi Jan, Sure! And sorry for posting these metrics for a while (we had other patches mentioning them) without proper explanation. We simulate a bunch of completions at random points in random files from our internal codebase. We assume the desired completion item

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

ilya-biryukov added a comment. @ioeric is the author of these completion metrics and evaluation tools. Eric, please feel free to correct me if I got something wrong or missed something. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59300/new/ https://reviews.llvm

[PATCH] D59354: [clangd] Print arguments in template specializations

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM with a few NITs. Thanks for fixing this! Comment at: clang-tools-extra/clangd/AST.cpp:86 +auto TL = Cls->getTypeAsWritten()->getTypeLoc(); +if (aut

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

ilya-biryukov added a comment. In D59302#1431274 , @kadircet wrote: > The optional part is rather limiting number of diagnostics that is going to > be surfaced. So if you have thousands of errors inside preamble only first > 100 of them will appear insid

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

ilya-biryukov added inline comments. Comment at: clangd/ClangdUnit.cpp:256 +// directive inside main file. +// If a header is transitively included in multiple direct includes of main, we +// choose the first one. We should find a way to point into an exact `incl

[PATCH] D59354: [clangd] Print arguments in template specializations

ilya-biryukov added inline comments. Comment at: clang/lib/AST/TypePrinter.cpp:1635 +static void printArgument(const TemplateArgument &A, const PrintingPolicy &PP, + llvm::raw_ostream &OS) { ilya-biryukov wrote: > NIT: consider also add

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:170 I.first->second = mergeSymbol(I.first->second, Sym); +// We re-count number of references while merging refs from scratch. +I.first->getSecond().References

[PATCH] D59599: [clangd] Fix a crash while printing Template Arguments

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: clangd/AST.cpp:95 +} else { + // FIXME: Cls->getTypeAsWritten might return null in some cases, e.g. + // clang sees first sees a

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

ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:135 +// \endcode +class RewriteRule { +public: ymandel wrote: > ilya-biryukov wrote: > > Maybe consider separating the fluent API to build the rewrite rule from

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

ilya-biryukov added a comment. Thanks for changes. A few more comments. Comment at: clangd/ClangdUnit.cpp:259 +// Generates a mapping from start of an include directive to its range. +std::map +positionToRangeMap(const std::vector &MainFileIncludes) { Could we

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/background-indexer/BackgroundIndexer.cpp:13 + +#include "/usr/local/google/home/kadircet/repos/llvm/clang-tools-extra/clangd/Context.h" +#include "/usr/local/google/home/kadircet/repos/llvm/clang-tools-ex

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

ilya-biryukov created this revision. ilya-biryukov added a reviewer: ioeric. Herald added a subscriber: jdoerfert. Herald added a project: clang. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D59683 Files: clang/lib/Tooling/AllTUsExecution.cpp Index: clang/lib/Tooling/AllTUs

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

ilya-biryukov planned changes to this revision. ilya-biryukov added a comment. This breaks lots of stuff, need to figure out why this happens :-( 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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/unittests/clangd/ASTUtilsTests.cpp:1 +#include "AST.h" +#include "Annotations.h" NIT: add a licence header Comment at: clang-tools-extra/unittests/clangd/CMakeLists.txt:13 An

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/AST.cpp:26 +llvm::Optional> +getTemplateSpecializationArgLocs(const NamedDecl &ND) { + if (auto *Func = llvm::dyn_cast(&ND)) { Eugene.Zelenko wrote: > Functions should be static, not in an

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

ilya-biryukov added a comment. False alarm, I was running tests on top of a previously broken revision. Everything seems to work just fine. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59683/new/ https://reviews.llvm.org/D59683

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

ilya-biryukov added a comment. I've just realized the review is probably missing the latest revision. @ymandel, could you upload the new version? Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:54 +/// boolean expression language for constructing filters. +cla

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

ilya-biryukov added inline comments. Comment at: clang/unittests/Tooling/TransformerTest.cpp:157 + .as() + .replaceWith(text("REPLACED")) + .because(text("Use size() method directly on string.")); NIT: maybe consider adding overloads for these func

[PATCH] D59757: [clangd] Send empty diagnostics when a file is closed

ilya-biryukov created this revision. ilya-biryukov added a reviewer: hokein. Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, ioeric. Herald added a project: clang. The LSP clients cannot know clangd will not send diagnostic updates for closed files, so we send them an empty list of

[PATCH] D59757: [clangd] Send empty diagnostics when a file is closed

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1000 std::lock_guard Lock(FixItsMutex); FixItsMap[File] = LocalFixIts; } hokein wrote: > IIUC, it seems that

[PATCH] D57943: [clangd] **Prototype**: clang-tidy-based tweaks

ilya-biryukov added a comment. Herald added a subscriber: jdoerfert. Another alternative we could consider would be collecting these fixes when producing the clang-tidy diagnostics. We won't show the diagnostics to the users if they have the check disabled, but we would stash the ranges and the

[PATCH] D59757: [clangd] Send empty diagnostics when a file is closed

ilya-biryukov updated this revision to Diff 192062. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. - Added a comment that races are not possible Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59757/new/ https://review

[PATCH] D59757: [clangd] Send empty diagnostics when a file is closed

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1000 std::lock_guard Lock(FixItsMutex); FixItsMap[File] = LocalFixIts; } hokein wrote: > ilya-biryukov wrote: > > hokein wrote: > > > IIUC, it seems that we

[PATCH] D59759: [clangd] Add .cu files to VSCode extension

ilya-biryukov created this revision. ilya-biryukov added a reviewer: hokein. Herald added subscribers: jdoerfert, kadircet, arphaman, jkorous, MaskRay, ioeric. Herald added a project: clang. clangd should be able to handle those with a proper compilation database. However, users using 'nvcc' migh

[PATCH] D59759: [clangd] Add .cu files to VSCode extension

ilya-biryukov added a comment. In D59759#1441321 , @hokein wrote: > Do we need to add an entry `"onLanguage:cuda"` for cuda in the extension > package.json? Yeah, sounds reasonable, but I don't see cuda in the list of languages in VSCode, nor does it t

[PATCH] D59759: [clangd] Add .cu files to VSCode extension

ilya-biryukov added a comment. In D59759#1441402 , @hokein wrote: > Taking a closer look, it seems cuda is not supported by default in vscode :( > > vscode provides a way to add a new language in the extension, see > https://code.visualstudio.com/api/refe

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

ilya-biryukov added a comment. In D59376#1439928 , @ymandel wrote: > Working on the new version now. Will note with "PTAL" once that's ready. Got you! Waiting for the new revision. > Sorry that wasn't clear in earlier responses. Or maybe it's me misrea

[PATCH] D54799: [clangd] textDocument/SymbolInfo method

ilya-biryukov added a comment. In D54799#1441409 , @thakis wrote: > SymbolInfoTests.All takes > 20% of all of check-clang-tools time. Can the > test be made faster? > > Also, since it's using a large-ish non-pod initializer clang is currently > slow to c

[PATCH] D59759: [clangd] Add .cu files to VSCode extension

ilya-biryukov updated this revision to Diff 192109. ilya-biryukov added a comment. - Added a comments about CUDA files Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59759/new/ https://reviews.llvm.org/D59759 Files: clang-tools-extra/clangd/clien

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

ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:89 + /// relevant name, not including qualifiers. + Name, +}; ymandel wrote: > ilya-biryukov wrote: > > Same here, what happens to the template arguments and

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

ilya-biryukov created this revision. ilya-biryukov added a reviewer: gribozavr. Herald added subscribers: cfe-commits, jdoerfert, kadircet, arphaman, jkorous, MaskRay, ioeric, hiraditya, mgorny. Herald added projects: clang, LLVM. Annotations allow writing nice-looking unit test code when one nee

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

ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. Seeking feedback for this change, mostly interested if this a good place for test helpers like this? I'm planning to author more tests in `clangTooling` using this soon, so moving it somewhere clang-specific is an opt

[PATCH] D59817: [clangd] Add activate command to the vscode extension.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM! Thanks! Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59817/new/ https://reviews.llvm.org/D59817 _

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

ilya-biryukov created this revision. ilya-biryukov added a reviewer: rsmith. Herald added a project: clang. By adding a hook to consume all tokens produced by the preprocessor. The intention of this change is to make it possible to consume the expanded tokens without re-runnig the preprocessor wit

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

ilya-biryukov created this revision. ilya-biryukov added a reviewer: gribozavr. Herald added subscribers: jdoerfert, mgorny. Herald added a project: clang. TokenBuffer stores the list of tokens for a file obtained after preprocessing. This is a base building block for syntax trees, see [1] for the

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

ilya-biryukov added a comment. I'm happy to assess the performance costs of this change if needed (the function is obviously on the hot path), but I hope this won't add any significant performance costs. Also happy to consider alternative approaches to collect the tokens when preprocessing, the

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

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:130 + /// result. + llvm::ArrayRef expansions() const { return Expansions; } + /// Tokens of macro directives and top-level macro e

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

ilya-biryukov marked 2 inline comments as done. ilya-biryukov added inline comments. Comment at: clang/lib/Tooling/Syntax/TokenBuffer.cpp:323 + PP.getLangOpts(), Tokens); + auto *CB = CBOwner.get(); + Eugene.Zelenko w

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

ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. Mostly nits and small API questions form my side. This looks very good overall! I'm planning to take a closer look at the handling of macros and various AST nodes in more detail this week, but the approach looks solid

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

ilya-biryukov added a comment. 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, the current behavior (waiting for the first preamble) actually seems like a better

[PATCH] D59927: [clangd] Support UTF-32 (i.e. codepoint) offsets.

ilya-biryukov added a reviewer: gribozavr. ilya-biryukov added a subscriber: gribozavr. ilya-biryukov added a comment. NIT: maybe remove parentheses from the change description, they seem to only add noise. Adding @gribozavr who definitely knows more about Unicode :-) Repository: rCTE Clang T

[PATCH] D59927: [clangd] Support UTF-32 (i.e. codepoint) offsets.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: unittests/clangd/SourceCodeTests.cpp:164 + llvm::Failed()); // out of range + // middle line + EXPECT_THAT_EXPECTED(pos

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

ilya-biryukov updated this revision to Diff 192660. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Rename macro expansion to macro invocation everywhere - Tweak comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/

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

ilya-biryukov updated this revision to Diff 192661. ilya-biryukov added a comment. - s/macroMacroInvocation/something else... 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] D59814: [Testing] Move clangd::Annotations to llvm testing support

ilya-biryukov updated this revision to Diff 192673. ilya-biryukov marked 4 inline comments as done. ilya-biryukov added a comment. - Move Range into the body of Annotations - Use triple-slash comments - Added a FIXME that we might want to change the syntax - Move the doc comment to the class Rep

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

ilya-biryukov added a comment. I've added a FIXME to the class. Also want to add some basic tests before landing this. Comment at: clang-tools-extra/unittests/clangd/Annotations.h:8 //===--===// -// -// Annot

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

ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. A few naming alternatives, will update the review after addressing other comments. Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:72 + /// macro or a name, arguments and parentheses o

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

ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. The only real question I have is about returning an error vs an empty transformation in of macros. The rest are just NITs. Thanks for the change! I'll get to the NodeId patch right away :-) Comment

[PATCH] D59329: [LibTooling] Add NodeId, a strong type for AST-matcher node identifiers.

ilya-biryukov added a subscriber: klimek. ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Refactoring/NodeId.h:29 +public: + explicit NodeId(std::string Id) : Id(std::move(Id)) {} + What are the use-cases for passing a custom id to t

[PATCH] D59329: [LibTooling] Add NodeId, a strong type for AST-matcher node identifiers.

ilya-biryukov added a comment. Could you provide the rationale for having `NodeID` vs just using strings for the binds? Is this just a more type-safe way to do the same thing or is that actually required to solve a particular problem? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST A

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

ilya-biryukov added a comment. Still have a few comments to address in `TokenCollector` and wrt to naming. But apart from this revision is ready for another round. Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:120 + /// the original source file. The tranformati

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

ilya-biryukov updated this revision to Diff 192839. ilya-biryukov marked 19 inline comments as done and 2 inline comments as done. ilya-biryukov added a comment. - Rename various things. - Update doc comments. - Search tokens in the tests by spelling, not by kind. - Add more tests. - Fix typos. -

[PATCH] D60040: [clangd] Use capacity() instead of size() in RefSlab::bytes()

ilya-biryukov added a comment. I'll take the courtesy and land it since @gribozavr is OOO today. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60040/new/ https://reviews.llvm.org/D60040 ___ cfe-commits

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

ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. Looks neat, thanks! A final set of NITs from my side, I don't think any of the comments have to do with the actual design of the library, so expect LGTM in the next round. Comment at: clang/include/

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

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. Looks great, thanks! Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:110 + // constructed with the builder class. + static constexpr char Ro

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

ilya-biryukov added a comment. Will address the rest of the comments later, answering a few questions that popped up first. Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:72 + /// macro or a name, arguments and parentheses of a function-like macro. + llvm::Arra

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

ilya-biryukov added a comment. Leaving a few first comments, have to run. Will take another look later, sorry about the delay today (also feel free to add more reviewers in case this is urgent). Comment at: clangd/ClangdServer.cpp:197 return CB(llvm::make_error()); +

[PATCH] D60179: [clangd] Return clangd::TextEdit in ClangdServer::rename. NFC

ilya-biryukov created this revision. ilya-biryukov added reviewers: hokein, ioeric, sammccall. Herald added subscribers: kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. Instead of tooling::Replacement. To avoid the need to have contents of the file at the caller site. This als

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

ilya-biryukov accepted this revision. ilya-biryukov added a comment. LGTM with the new changes. Specifying the `clang::Expr` type explicitly when calling `change` looks a bit less clear than the original approach, but also looks pretty clear to me. Repository: rG LLVM Github Monorepo CHANGE

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