[PATCH] D37101: [clangd] Add support for snippet completions

ilya-biryukov added a comment. Looks good overall, just a bunch of comments before accepting the revision. Mostly minor code style issues, sorry for spamming you with those. Comment at: clangd/ClangdLSPServer.h:30 ClangdLSPServer(JSONOutput &Out, unsigned AsyncThreadsCount,

[PATCH] D37282: clangd: Tolerate additional headers

ilya-biryukov added a comment. In https://reviews.llvm.org/D37282#860239, @puremourning wrote: > No I don't have commit rights. Can you land for me? Thanks! Done. Repository: rL LLVM https://reviews.llvm.org/D37282 ___ cfe-commits mailing list

[PATCH] D37474: [PCH] Allow VFS to be used for tests that generate PCH files

ilya-biryukov added a comment. Could we fix this in tests instead by providing an overlay over `RealFS` there instead of doing that in `ASTUnit`? If I'm passing `vfs::FileSystem`, I specifically **do not want** any file accesses to silently go through `RealFS`, ignoring the `vfs::FileSystem` th

[PATCH] D37474: [PCH] Allow VFS to be used for tests that generate PCH files

ilya-biryukov added a comment. In https://reviews.llvm.org/D37474#861206, @cameron314 wrote: > I suppose we could do the overlay manually in all the tests that need it; I > guess it depends on what the goal of the VFS support is. To my mind, it > doesn't make sense that a file is placed in the

[PATCH] D37491: [Preamble] Fixed preamble breaking with BOM presence (and particularly, fluctuating BOM presence)

ilya-biryukov removed a reviewer: cfe-commits. ilya-biryukov added subscribers: cfe-commits, klimek, bkramer. ilya-biryukov added a comment. - How are various preprocessor offests (and `SourceLocation` offsets) are calculated? Do they account for `BOM` presence and ignore it? - Are there potentia

[PATCH] D37101: [clangd] Add support for snippet completions

ilya-biryukov added a comment. Looks good, just one major drawback to address. We shouldn't break clients by default (see comment in `ClangdMain.cpp`). And a minor comment about flag naming. Comment at: clangd/tool/ClangdMain.cpp:30 +static llvm::cl::opt +DisableSnippets("

[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path

ilya-biryukov added a comment. See my comments. Could you also `clang-format` the code please? Comment at: clangd/ClangdServer.cpp:150 bool RunSynchronously, + std::string CompileCommands, llvm::O

[PATCH] D37101: [clangd] Add support for snippet completions

ilya-biryukov added inline comments. Comment at: clangd/ClangdUnit.cpp:376 + +CompletionItem Item{InsertTextFormat::PlainText}; + rwols wrote: > ilya-biryukov wrote: > > Implementations of this function in `PlainTextCompletionItemsCollector` and > > `Snippet

[PATCH] D37491: [Preamble] Fixed preamble breaking with BOM presence (and particularly, fluctuating BOM presence)

ilya-biryukov added a comment. In https://reviews.llvm.org/D37491#862160, @cameron314 wrote: > > Are there potential problems we may run into because of the changing > > offsets? Could we add tests checking changing the offsets does not matter? > > That's a good point; I've looked into it and th

[PATCH] D37474: [PCH] Allow VFS to be used for tests that generate PCH files

ilya-biryukov added a comment. In https://reviews.llvm.org/D37474#862187, @cameron314 wrote: > I'll change the overlay to only allow access to the PCH. > > I agree that it would be nice to only read the PCH using real filesystem APIs > in order to be symmetrical, but this seems non-trivial. It a

[PATCH] D36150: [clangd] LSP extension to switch between source/header file

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. Just a few more comments. Should be easy to fix. Could you also please `clang-format` the code? Comment at: clangd/ClangdLSPServer.cpp:225 + llvm::Op

[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path

ilya-biryukov added a comment. Could you also `clang-format` the code please? Comment at: clangd/ClangdServer.cpp:150 bool RunSynchronously, + llvm::Optional CompileCommandsDir, llvm::Optional Res

[PATCH] D37101: [clangd] Add support for snippet completions

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. Thanks for the change! Looks good minus an outdated description of `enable-snippets` flag. Do you have commit access to llvm repository? Comment at: clangd/too

[PATCH] D37491: [Preamble] Fixed preamble breaking with BOM presence (and particularly, fluctuating BOM presence)

ilya-biryukov added a comment. Parsing errors on preamble additions and removals are definitely bad and should be fixed. But I would argue that the right approach is to invalidate the preamble and rebuild it on BOM changes. Current fix in `ASTUnit` just hides an error in the underlying APIs. F

[PATCH] D37474: [PCH] Allow VFS to be used for tests that generate PCH files

ilya-biryukov added a comment. In https://reviews.llvm.org/D37474#863335, @cameron314 wrote: > Looking at the way remapped buffers are handled, I just remembered that they > must exist on the file system (at the very least, in a directory that exists) > or the remapping is not taken into accoun

[PATCH] D37382: Fixed a crash in code completion.

ilya-biryukov updated this revision to Diff 114349. ilya-biryukov added a comment. - Fix to account for change in the interface of ParseExpressionList. https://reviews.llvm.org/D37382 Files: lib/Parse/ParseDecl.cpp test/CodeCompletion/crash-func-init.cpp Index: test/CodeCompletion/crash-f

[PATCH] D37491: [Preamble] Fixed preamble breaking with BOM presence (and particularly, fluctuating BOM presence)

ilya-biryukov added a comment. In https://reviews.llvm.org/D37491#864649, @erikjv wrote: > Will this fix PR25023 and PR21144? PR25023 should be fixed by this change. It is essentially a repro of the same bug. Could we add a `c-index-test`-based tes

[PATCH] D37474: [PCH] Allow VFS to be used for tests that generate PCH files

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. Looks good, just a few minor style comments. Comment at: include/clang/Frontend/PrecompiledPreamble.h:100 /// PreambleBounds used to build the preamble

[PATCH] D37101: [clangd] Add support for snippet completions

ilya-biryukov added a comment. @rwols, do you have access to svn repo? If not, I can submit this change for you. https://reviews.llvm.org/D37101 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/

[PATCH] D36150: [clangd] LSP extension to switch between source/header file

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. Overall the code looks good, thanks for the clean-ups. Only a few minor style comments and a major one about returning value when no file is matching. Please add tests

[PATCH] D37700: Fix recording preamble's conditional stack in skipped PP branches.

ilya-biryukov created this revision. This fixes PR34547. `Lexer::LexEndOfFile` handles recording of ConditionalStack for preamble and reporting errors about unmatched conditionalal PP directives. However, SkipExcludedConditionalBlock contianed duplicated logic for reporting errors and clearing Con

[PATCH] D37700: Fix recording preamble's conditional stack in skipped PP branches.

ilya-biryukov updated this revision to Diff 114634. ilya-biryukov added a comment. Fixed description of the change. https://reviews.llvm.org/D37700 Files: lib/Lex/PPDirectives.cpp test/Index/preamble-conditionals-inverted-with-error.cpp test/Index/preamble-conditionals-inverted.cpp Inde

[PATCH] D37700: Fix recording preamble's conditional stack in skipped PP branches.

ilya-biryukov added a comment. In https://reviews.llvm.org/D37700#867625, @erikjv wrote: > I'd put/fix Nik's issue in a separate patch. Totally agree. It seems like a separate issue, though maybe related. @nik, could you file a separate bug so that we won't forget about it? Repository: rL

[PATCH] D37101: [clangd] Add support for snippet completions

ilya-biryukov added a comment. Done. It's now in. Thanks for the contribution! Had to make a few changes before committing. - Fix compilation of tests. - Update VSCode "engine" dependency in vscode toy client. https://reviews.llvm.org/D37101 ___

[PATCH] D36150: [clangd] LSP extension to switch between source/header file

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. Overall looks good, but still needs at least a few tests. https://reviews.llvm.org/D36150 ___ cfe-commits mailing list cf

[PATCH] D37101: [clangd] Add support for snippet completions

ilya-biryukov closed this revision. ilya-biryukov added a comment. Marking as closed, had to commit by hand without `arc patch` as it couldn't find base revision to apply the patch on. https://reviews.llvm.org/D37101 ___ cfe-commits mailing list cf

[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path

ilya-biryukov added inline comments. Comment at: clangd/GlobalCompilationDatabase.cpp:75 + if (CachedIt != CompilationDatabases.end()) +return (CachedIt->second.get()); + auto CDB = tooling::CompilationDatabase::loadFromDirectory(File, Error); Parentheses s

[PATCH] D37101: [clangd] Add support for snippet completions

ilya-biryukov added a comment. In https://reviews.llvm.org/D37101#868207, @rwols wrote: > Thanks! Thinking ahead now so we're on the same page, you will be > implementing the client's initialize request, and I'll start work on > textDocument/signatureHelp. Sounds good. https://reviews.llvm.

[PATCH] D37491: [Preamble] Fixed preamble breaking with BOM presence (and particularly, fluctuating BOM presence)

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. See my comments about removing `StartOffset` field, but other than that looks good. Comment at: include/clang/Lex/Lexer.h:52 + /// a BOM is present at the sta

[PATCH] D37491: [Preamble] Fixed preamble breaking with BOM presence (and particularly, fluctuating BOM presence)

ilya-biryukov added inline comments. Comment at: include/clang/Lex/Lexer.h:52 + /// a BOM is present at the start of the file. + unsigned StartOffset; + /// \brief Size of the preamble in bytes. cameron314 wrote: > ilya-biryukov wrote: > > We could simplify it

[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path

ilya-biryukov added a comment. All lit test for clangd are currently failing if I apply your change. Generally, please run `make check-clang-tools` to make sure your changes do not introduce any regressions. The tests are failing because when `--compile-commands-dir` is not specified clangd now

[PATCH] D37970: [clangd] Added a command-line arg to mirror clangd input into a file.

ilya-biryukov created this revision. The arg is useful for debugging and creating test cases. https://reviews.llvm.org/D37970 Files: clangd/JSONRPCDispatcher.cpp clangd/JSONRPCDispatcher.h clangd/tool/ClangdMain.cpp test/clangd/input-mirror.test Index: test/clangd/input-mirror.test ===

[PATCH] D37970: [clangd] Added a command-line arg to mirror clangd input into a file.

ilya-biryukov updated this revision to Diff 115628. ilya-biryukov added a comment. Replaced std::string with Path typedef. https://reviews.llvm.org/D37970 Files: clangd/JSONRPCDispatcher.cpp clangd/JSONRPCDispatcher.h clangd/tool/ClangdMain.cpp test/clangd/input-mirror.test Index: test

[PATCH] D37972: [clangd] Introduced Logger interface.

ilya-biryukov created this revision. Herald added a subscriber: mgorny. This fixes a bunch of logging-related FIXMEs. https://reviews.llvm.org/D37972 Files: clangd/CMakeLists.txt clangd/ClangdLSPServer.cpp clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/ClangdUnit.cpp clangd/Cl

[PATCH] D37972: [clangd] Introduced Logger interface.

ilya-biryukov updated this revision to Diff 115648. ilya-biryukov marked 4 inline comments as done. ilya-biryukov added a comment. Addressed review comments. - Replaced include with forward declaration in headers. - Removed logging on each call to loadFromDirectory. https://reviews.llvm.org/D37

[PATCH] D37972: [clangd] Introduced Logger interface.

ilya-biryukov added inline comments. Comment at: clangd/GlobalCompilationDatabase.cpp:88 if (!Error.empty()) { -// FIXME(ibiryukov): logging -// Output.log("Error when trying to load compilation database from " + -//Twine(Path) + ": " +

[PATCH] D38032: [clangd] Serialize onDiagnosticsReady callbacks for the same file.

ilya-biryukov created this revision. Calls to onDiagnosticsReady were done concurrently before. This sometimes led to older versions of diagnostics being reported to the user after the newer versions. https://reviews.llvm.org/D38032 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h cla

[PATCH] D37972: [clangd] Introduced Logger interface.

ilya-biryukov updated this revision to Diff 115818. ilya-biryukov added a comment. - Replaced JSONOutput in Protocol.h/Protocol.cpp with Logger. https://reviews.llvm.org/D37972 Files: clangd/CMakeLists.txt clangd/ClangdLSPServer.cpp clangd/ClangdServer.cpp clangd/ClangdServer.h clangd

[PATCH] D38032: [clangd] Serialize onDiagnosticsReady callbacks for the same file.

ilya-biryukov updated this revision to Diff 115821. ilya-biryukov added a comment. - Removed an incidental `Mutex.unlock()`, a leftover from previous drafts. https://reviews.llvm.org/D38032 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/DraftStore.h unittests/clangd/ClangdT

[PATCH] D36150: [clangd] LSP extension to switch between source/header file

ilya-biryukov added a comment. In https://reviews.llvm.org/D36150#875623, @Nebiroth wrote: > In https://reviews.llvm.org/D36150#867971, @ilya-biryukov wrote: > > > Overall looks good, but still needs at least a few tests. > > > I have a test for this commit that uses included source and header fi

[PATCH] D38048: [clangd] Add textDocument/signatureHelp

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. Thanks for the patch. Overall looks good, just a few comments. Could you also add some tests? Comment at: clangd/ClangdServer.h:243 + /// Provide sig

[PATCH] D38032: [clangd] Serialize onDiagnosticsReady callbacks for the same file.

ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.cpp:321-324 +// FIXME(ibiryukov): get rid of '<' comparison here. In the current +// implementation diagnostics will not be reported after version counters' +// overflow. This should not happen in pr

[PATCH] D38077: [clangd] Put inacessible items to the end of completion list.

ilya-biryukov created this revision. https://reviews.llvm.org/D38077 Files: clangd/ClangdUnit.cpp test/clangd/authority-less-uri.test test/clangd/completion-priorities.test test/clangd/completion-snippet.test test/clangd/completion.test test/clangd/protocol.test Index: test/clangd/pr

[PATCH] D38032: [clangd] Serialize onDiagnosticsReady callbacks for the same file.

ilya-biryukov updated this revision to Diff 115985. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Added a comment to version map. - Fixed compilation after rebase. https://reviews.llvm.org/D38032 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clang

[PATCH] D38032: [clangd] Serialize onDiagnosticsReady callbacks for the same file.

ilya-biryukov added a comment. Thanks for the review! https://reviews.llvm.org/D38032 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38081: Set completion priority of destructors and operators to CCP_Unlikely.

ilya-biryukov created this revision. Herald added a subscriber: eraman. It will move destructors and operators to the end of completion list. Destructors and operators are currently very high on the completion list, as they have the same priority as member functions. However, they are clearly not

[PATCH] D38083: [clangd] Skip informative qualifier chunks.

ilya-biryukov created this revision. Completion results look much nicer without them. Informative qualifiers are stored for every method from a base class, even when calling those methods does not require any qualifiers. For example, struct Foo { int foo(); }; struct Bar : Foo { }; void tes

[PATCH] D38077: [clangd] Put inacessible items to the end of completion list.

ilya-biryukov added a comment. Thanks for quick review! https://reviews.llvm.org/D38077 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38083: [clangd] Skip informative qualifier chunks.

ilya-biryukov updated this revision to Diff 116297. ilya-biryukov added a comment. - Fixed CHECK-DAG typo. https://reviews.llvm.org/D38083 Files: clangd/ClangdUnit.cpp test/clangd/completion-qualifiers.test Index: test/clangd/completion-qualifiers.test

[PATCH] D38083: [clangd] Skip informative qualifier chunks.

ilya-biryukov added inline comments. Comment at: test/clangd/completion-qualifiers.test:12 +# CHECK: {"jsonrpc":"2.0","id":2,"result":[ +# CHEKC-DAG: {"label":"foo() const","kind":2,"detail":"int","sortText":"00035foo","filterText":"foo","insertText":"foo","insertTextFormat":1}

[PATCH] D36150: [clangd] LSP extension to switch between source/header file

ilya-biryukov accepted this revision. ilya-biryukov added a comment. Looks good modulo a few NITS. Let me know if you need help landing this. Comment at: unittests/clangd/ClangdTests.cpp:910 + auto FooH = getVirtualTestFilePath("foo.h"); + auto invalid = getVirtualTestFilePat

[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path

ilya-biryukov added a comment. Could you please run `clang-format` on every submission? Comment at: clangd/GlobalCompilationDatabase.cpp:98 + { +CompileCommandsDir = "/"; +return tryLoadDatabaseFromPath(CompileCommandsDir.getValue()); Is this some

[PATCH] D38081: Set completion priority of destructors and operators to CCP_Unlikely.

ilya-biryukov added a comment. Friendly ping. Any suggestions for other reviewers are also very welcome. https://reviews.llvm.org/D38081 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi

[PATCH] D38081: Set completion priority of destructors and operators to CCP_Unlikely.

ilya-biryukov updated this revision to Diff 116381. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Removed redundant NameKind:: qualifiers. https://reviews.llvm.org/D38081 Files: lib/Sema/SemaCodeComplete.cpp test/Index/complete-access-checks.cpp test/Ind

[PATCH] D38081: Set completion priority of destructors and operators to CCP_Unlikely.

ilya-biryukov added a comment. Thanks for the review! Sorry for inappropriate ping, I'll make sure to stick to the policy. https://reviews.llvm.org/D38081 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/

[PATCH] D36150: [clangd] LSP extension to switch between source/header file

ilya-biryukov accepted this revision. ilya-biryukov added a comment. Looks good. Do you want me to submit this patch for you? https://reviews.llvm.org/D36150 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailm

[PATCH] D62616: [CodeComplete] Add a bit more whitespace to completed patterns

ilya-biryukov added a comment. In D62616#1522284 , @gribozavr wrote: > I totally agree about the space before the opening curly, but the space > before the opening paren is a matter of style. Certainly it does match LLVM > style better, but it does not

[PATCH] D62582: [CodeComplete] Improve overload handling for C++ qualified and ref-qualified methods.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: lib/Sema/SemaCodeComplete.cpp:1314 + Result &Incumbent = Results[Entry.second]; + Method->dumpColor(); + Incumbent.D

[PATCH] D61637: [Syntax] Introduce syntax trees

ilya-biryukov updated this revision to Diff 202745. ilya-biryukov marked 24 inline comments as done. ilya-biryukov added a comment. Herald added a project: LLVM. Herald added a subscriber: llvm-commits. - Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https

[PATCH] D61637: [Syntax] Introduce syntax trees

ilya-biryukov added a comment. I've addressed most of the comments, except the naming ones. We need a convention for naming the language nodes and names for composite and leaf structural nodes. For "language" nodes, I suggest we use `CompoundStatement`, `Recovery`, `TopLevelDeclaration`, `Temp

[PATCH] D61637: [Syntax] Introduce syntax trees

ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Syntax/BuildTree.h:11 +// +// The code is divided in the following way: +// - 'Tree/Cascade.h' defines the basic structure of the syntax tree, This needs an update, will do in the next

[PATCH] D62814: [clangd] Treat lambdas as functions when preparing hover response

ilya-biryukov added a comment. Do we care about pointers or references to lambda types? Comment at: clang-tools-extra/clangd/XRefs.cpp:662 + if (const DecltypeType *DT = + llvm::dyn_cast(VD->getType().getTypePtr())) +if (!DT->getUnderlyingType().isNull

[PATCH] D62621: [LibTooling] Add insert/remove convenience functions for creating `ASTEdit`s.

ilya-biryukov added a comment. Are there any corner cases with handling whitespace that we might want to handle or document them if we don't handle them? E.g. if the range ends up being a name of an expanded macro , it's probably very easy to glue the macro name with whatever you're inserting.

[PATCH] D62621: [LibTooling] Add insert/remove convenience functions for creating `ASTEdit`s.

ilya-biryukov added a comment. I guess the question also applies to `change`, although it did not occur to me before. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62621/new/ https://reviews.llvm.org/D62621 _

[PATCH] D62814: [clangd] Treat lambdas as functions when preparing hover response

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. In D62814#1528736 , @kadircet wrote: > Nice catch! I think it makes sense to show signature in those cases as well. > Updating according

[PATCH] D62814: [clangd] Treat lambdas as functions when preparing hover response

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. Please add a check for the type of variable being not null before landing. The other comment is not very important Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D62814: [clangd] Treat lambdas as functions when preparing hover response

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/XRefs.cpp:628 +auto QT = VD->getType(); +while (!QT->getPointeeType().isNull()) + QT = QT->getPointeeType(); We need a check for `!QT.isNull` here Comment at

[PATCH] D62856: [clangd] Also apply adjustArguments when returning fallback commands

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

[PATCH] D62814: [clangd] Treat lambdas as functions when preparing hover response

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:831 + }; return HI; }}, kadircet wrote: > ilya-biryukov wrote: > > kadircet wrote: > > > ilya-biryukov wrote: > > > > Could you add anothe

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

ilya-biryukov added a comment. @jkorous, could you take a look or suggest someone who can review the libclang changes? As mentioned before, I've reviewed the clang bits (which are now gone), but don't have much experience in `libclang` parts. Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D62621: [LibTooling] Add insert/remove convenience functions for creating `ASTEdit`s.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. In D62621#1529691 , @ymandel wrote: > Thanks for the review. > > Good questions. The answer to both is that it depends on the `RangeSelec

[PATCH] D62476: [clangd] Support offsets for parameters in signatureHelp

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/trunk/clangd/CodeComplete.cpp:925 +// FIXME: this should only be set on CK_CurrentParameter. +Signal.ContainsActiveParameter = true; + } ua

[PATCH] D62476: [clangd] Support offsets for parameters in signatureHelp

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/trunk/clangd/CodeComplete.cpp:925 +// FIXME: this should only be set on CK_CurrentParameter. +Signal.ContainsActiveParameter = true; + } il

[PATCH] D62953: [Syntax] Do not glue multiple empty PP expansions to a single mapping

ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added a project: clang. This change makes sure we have a single mapping for each macro expansion, even if the result of expansion was empty. To achieve that, we take information from PPCallbacks::MacroExpands i

[PATCH] D62954: [Syntax] Add a helper to find expansion by its first spelled token

ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added a subscriber: kadircet. Herald added a project: clang. Used in clangd for a code tweak that expands a macro. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D62954 Files: clang/include

[PATCH] D62956: [clangd] Collect tokens of main files when building the AST

ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, mgorny. Herald added a project: clang. ilya-biryukov added a child revision: D61681: [clangd] A code tweak to expand a macro. The first use of this is a

[PATCH] D61681: [clangd] A code tweak to expand a macro

ilya-biryukov updated this revision to Diff 203358. ilya-biryukov added a comment. - Move logically separate parts to other changes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61681/new/ https://reviews.llvm.org/D61681 Files: clang-tools-extra

[PATCH] D61697: [lit] Disable test on darwin when building shared libs.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM, I see no way around it, we can't copy all of the shared libs. Maybe there's an alternative way to test this without copying the binary, but I don't have one in mind. Repos

[PATCH] D62804: [clangd] Enable extraction of system includes from custom toolchains

ilya-biryukov added a comment. Could you give more context on what the custom toolchains are? One feasible alternative is to move this detection to clang's driver (where the rest of include path detection lives), why won't that work? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACT

[PATCH] D62999: [clangd] Return 'RequestCancelled' on spurious completion triggers

ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added subscribers: kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. We currently return 'UnknownErrorCode' and it causes `coc.nvim` to show an error message to the user, which we want to avoi

[PATCH] D62999: [clangd] Return empty results on spurious completion triggers

ilya-biryukov marked 2 inline comments as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:734 +"ignored auto-triggered completion, preceding char did not match", +ErrorCode::RequestCancelled)); Server->codeCom

[PATCH] D62999: [clangd] Return empty results on spurious completion triggers

ilya-biryukov updated this revision to Diff 203567. ilya-biryukov added a comment. - Return empty results instead of an error Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62999/new/ https://reviews.llvm.org/D62999 Files: clang-tools-extra/clang

[PATCH] D62814: [clangd] Treat lambdas as functions when preparing hover response

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:831 + }; return HI; }}, kadircet wrote: > ilya-biryukov wrote: > > kadircet wrote: > > > i

[PATCH] D63098: [CodeComplete] Allow completing enum values within case statements, and insert 'case' as a fixit.

ilya-biryukov added a comment. Not displaying 'case' is actually confusing to my taste. WDYT about allowing multiple "typed text" chunks or allowing to mark other chunks that are supposed be part of filter text? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63098

[PATCH] D63091: [clangd] Add a capability to enable completions with fixes.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. NIT: I've been pushing towards giving clangd-specific extensions names that would absolutely never clash with anything that LSP proposes. In particular, the file status notificat

[PATCH] D63140: [clangd] Return TextEdits from ClangdServer::applyTweak

ilya-biryukov created this revision. ilya-biryukov added a reviewer: kadircet. Herald added subscribers: arphaman, jkorous, MaskRay. Herald added a project: clang. Instead of tooling::Replacements. So that embedders do not need to store the contents of the file. This also aligns better ClangdServ

[PATCH] D62804: [clangd] Enable extraction of system includes from custom toolchains

ilya-biryukov added a comment. In D62804#1538155 , @kadircet wrote: > For example a gcc cross compiling to arm comes with its own system includes > and has some mechanisms to discover that implicitly, without requiring any > "-I" flags. > Hence when use

[PATCH] D63098: [CodeComplete] Allow completing enum values within case statements, and insert 'case' as a fixit.

ilya-biryukov added a comment. Notes from the offline discussion: - Sam: supporting multiple typed text chunks is hard, many clients that expect only one. Objective-C already does that, though, but in a way that does not break most targets. - Sam: supporting any other form of custom text for fi

[PATCH] D63188: Fixed a crash in misc-redundant-expression ClangTidy checker

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

[PATCH] D63193: [clangd] Fix typo in GUARDED_BY()

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

[PATCH] D63140: [clangd] Return TextEdits from ClangdServer::applyTweak

ilya-biryukov added a comment. @hokein just realized you might be a better reviewer, since this makes `applyTweak` aligned with `rename`. And you implemented `rename` in the first place Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63140/new/ htt

[PATCH] D63194: [clangd] Link in target infos and pass target and mode while invoking driver

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:36 Cmd.CommandLine = ArgsAdjuster(Cmd.CommandLine, Cmd.Filename); + tooling::addTargetAndModeForProgramName(Cmd.CommandLine, Cmd.CommandLine[0]); // Inject the resource

[PATCH] D63194: [clangd] Link in target infos and pass target and mode while invoking driver

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:36 Cmd.CommandLine = ArgsAdjuster(Cmd.CommandLine, Cmd.Filename); + tooling::addTargetAndModeForProgramName(Cmd.CommandLine, Cmd.CommandLine[0]); // Inject the resource

[PATCH] D63194: [clangd] Link in target infos and pass target and mode while invoking driver

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/test/target_info.test:8 +# (with the extra slash in the front), so we add it here. +# RUN: sed -e "s|file://\([A-Z]\):/|file:///\1:/|g" %t.test.1 > %t.test +

[PATCH] D61637: [Syntax] Introduce syntax trees

ilya-biryukov updated this revision to Diff 204488. ilya-biryukov marked 7 inline comments as done. ilya-biryukov added a comment. - A leaf node stores a single token - Restructure code to avoid special-casing leaf nodes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://

[PATCH] D61637: [Syntax] Introduce syntax trees

ilya-biryukov added a comment. This is not 100% ready yet, but wanted to send it out anyway, as I'll be on vacation until Tuesday. I've addressed most major comments. In particular, `TreeBuilder` now looks simpler (and more structured) to my taste. One thing that's missing is adding children i

[PATCH] D63194: [clangd] Link in target infos and pass target and mode while invoking driver

ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. Nice, thanks! The clangd parts obviously LGTM, but let's be more conservative with the driver bits (see the relevant comment) Comment at: clang-tools-extra/clangd/test/target_info.test:28 +} +# Make

[PATCH] D62538: [clangd] Add hidden tweaks to dump AST/selection.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM with a few NITs Comment at: clangd/refactor/Tweak.h:104 + /// Is this a 'hidden' tweak, which are off by default. + virtual bool hidden() const { return

[PATCH] D62954: [Syntax] Add a helper to find expansion by its first spelled token

ilya-biryukov updated this revision to Diff 205352. ilya-biryukov marked 7 inline comments as done. ilya-biryukov added a comment. - Address comments. - s/findExpansion/expansionStartingAt. - Add tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D62954: [Syntax] Add a helper to find expansion by its first spelled token

ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:209 + ///#define FOO 1 2 3 // Expansion from "#define FOO 1" to an empty range. + ///FOO // Expansion from "FOO" to "1 2 3". + struct Expansion {

<    11   12   13   14   15   16   17   18   19   20   >