ilya-biryukov (Ilya Biryukov)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 6 2017, 1:42 AM (70 w, 5 d)

Recent Activity

Today

ilya-biryukov updated the diff for D50727: [clangd] Fetch documentation from the Index during signature help.
  • run clang-format
Tue, Aug 14, 11:17 AM
ilya-biryukov added a dependency for D50727: [clangd] Fetch documentation from the Index during signature help: D50726: [clangd] Show function documentation in sigHelp.
Tue, Aug 14, 11:15 AM
ilya-biryukov added a dependent revision for D50726: [clangd] Show function documentation in sigHelp: D50727: [clangd] Fetch documentation from the Index during signature help.
Tue, Aug 14, 11:15 AM
ilya-biryukov removed a dependent revision for D50727: [clangd] Fetch documentation from the Index during signature help: D50726: [clangd] Show function documentation in sigHelp.
Tue, Aug 14, 11:15 AM
ilya-biryukov removed a dependency for D50726: [clangd] Show function documentation in sigHelp: D50727: [clangd] Fetch documentation from the Index during signature help.
Tue, Aug 14, 11:15 AM
ilya-biryukov added a dependency for D50726: [clangd] Show function documentation in sigHelp: D50727: [clangd] Fetch documentation from the Index during signature help.
Tue, Aug 14, 11:15 AM
ilya-biryukov added a dependent revision for D50727: [clangd] Fetch documentation from the Index during signature help: D50726: [clangd] Show function documentation in sigHelp.
Tue, Aug 14, 11:15 AM
ilya-biryukov added a comment to D50726: [clangd] Show function documentation in sigHelp.

There's a test for the new behavior in D50726

Tue, Aug 14, 11:14 AM
ilya-biryukov created D50727: [clangd] Fetch documentation from the Index during signature help.
Tue, Aug 14, 11:13 AM
ilya-biryukov created D50726: [clangd] Show function documentation in sigHelp.
Tue, Aug 14, 11:02 AM
ilya-biryukov accepted D50571: [clangd] add an extension field to LSP to transfer the diagnostic's category.

LGTM. Let's watch out for possible breakages in any of the clients, though. I've checked VSCode and it seems to be fine with the added fields.

Tue, Aug 14, 10:33 AM
ilya-biryukov added a comment to D50641: [clangd][test] Fix exit messages in tests.

Haven't noticed Alex's comments, sorry for a duplicate suggestion about exiting with failure error code

Tue, Aug 14, 8:18 AM · Restricted Project
ilya-biryukov accepted D50641: [clangd][test] Fix exit messages in tests.

LGTM. Many thanks for fixing this.

Tue, Aug 14, 8:09 AM · Restricted Project
ilya-biryukov added a comment to D50502: [clangd] Initial cancellation mechanism for LSP requests..

Sorry for missing this one. The biggest concern that I have is about the thread-safety of the API, it's too easy to misuse from multiple threads at this point.

Tue, Aug 14, 7:47 AM
ilya-biryukov added inline comments to D50645: [clangd] Show non-instantiated decls in signatureHelp.
Tue, Aug 14, 2:12 AM
ilya-biryukov updated the diff for D50645: [clangd] Show non-instantiated decls in signatureHelp.
  • Use 'auto*' instead of 'auto'
Tue, Aug 14, 2:12 AM

Yesterday

ilya-biryukov created D50645: [clangd] Show non-instantiated decls in signatureHelp.
Mon, Aug 13, 10:03 AM
ilya-biryukov added inline comments to D50438: [clangd] Sort GoToDefinition results..
Mon, Aug 13, 4:33 AM
ilya-biryukov accepted D50628: [Preamble] Empty preamble is not an error..

LGTM

Mon, Aug 13, 4:19 AM
ilya-biryukov added a comment to D50627: [clangd] Add a testcase for empty preamble..

Maybe also add a test for find-definition that was broken before? (non-empty preamble -> empty preamble -> bad gotodef that goes to included file instead of the local variable)
To have a regression test against similar failures.

Mon, Aug 13, 4:19 AM
ilya-biryukov added inline comments to D50571: [clangd] add an extension field to LSP to transfer the diagnostic's category.
Mon, Aug 13, 1:43 AM

Fri, Aug 10

ilya-biryukov added a comment to D50443: [clang] Store code completion token range in preprocessor..

NIT: maybe also note the number of the clangd revision in this change's description

Fri, Aug 10, 7:15 AM
ilya-biryukov accepted D50443: [clang] Store code completion token range in preprocessor..

LGTM, but let's land together with a dependent revision to hove some code that actually tests it.

Fri, Aug 10, 7:15 AM
ilya-biryukov accepted D50555: [clangd] Introduce scoring mechanism for SignatureInformations..

Thanks! LGTM with a few NITs

Fri, Aug 10, 7:10 AM
ilya-biryukov accepted D50431: generalize SKS key server in debian8 Dockerfile.

Sure, LGTM, did not want to block you.

Fri, Aug 10, 6:59 AM
ilya-biryukov accepted D50449: [clangd] Support textEdit in addition to insertText..

LGTM. Thanks for the change!
Could we add an option to clangd to switch it on? (VSCode does not work, but our hacked-up ycm integration seems to work, right?)

Fri, Aug 10, 5:38 AM
ilya-biryukov added inline comments to D50555: [clangd] Introduce scoring mechanism for SignatureInformations..
Fri, Aug 10, 5:24 AM
ilya-biryukov accepted D50415: [clangd] extend the publishDiagnostics response to send back fixits to the client directly as well (if requested).

LGTM with a small NIT.

Fri, Aug 10, 5:11 AM
ilya-biryukov added a reviewer for D48687: [clangd] Avoid duplicates in findDefinitions response: ilya-biryukov.
Fri, Aug 10, 5:08 AM
ilya-biryukov added a comment to D48687: [clangd] Avoid duplicates in findDefinitions response.

Only a few NITs from my side.
Excited for this fix to get in, have been seeing duplicate in other cases too :-)

Fri, Aug 10, 5:08 AM
ilya-biryukov added a comment to D50502: [clangd] Initial cancellation mechanism for LSP requests..

I have left a few comments, but wanted to start with a higher-level design consideration.

Fri, Aug 10, 3:25 AM

Thu, Aug 9

ilya-biryukov added inline comments to D50449: [clangd] Support textEdit in addition to insertText..
Thu, Aug 9, 10:23 AM
ilya-biryukov accepted D50446: [clangd] Record the file being processed in a TUScheduler thread in context..

LGTM. Thanks!

Thu, Aug 9, 2:00 AM
ilya-biryukov added inline comments to D50415: [clangd] extend the publishDiagnostics response to send back fixits to the client directly as well (if requested).
Thu, Aug 9, 1:29 AM
ilya-biryukov added a comment to D50431: generalize SKS key server in debian8 Dockerfile.

Maybe hard-code the SHA256 checksum directly into the script instead?
Going through the keyservers does not seem to buy us much in terms of security and we certainly don't update the Dockerfiles very often, so it does not take too much time to verify those SHA256 checksums by hand when we do.
Using the gpg was a mistake on my end, it makes things more complicated and less reliable.

Thu, Aug 9, 1:26 AM
ilya-biryukov added a comment to D50455: Continue emitting diagnostics after a fatal error.

The new behavior looks reasonable for interactive tools like clangd, but I'm a bit worried that clang may emit too many irrelevant errors, because it may not cope nicely with those fatal errors, i.e. wasn't tested that thoroughly in that mode.
Nevertheless, I think this is moving clangd in the right direction and if we see too many irrelevant errors, we should look into improving clang to show less of those.

Thu, Aug 9, 1:19 AM

Wed, Aug 8

ilya-biryukov added inline comments to D50446: [clangd] Record the file being processed in a TUScheduler thread in context..
Wed, Aug 8, 8:27 AM
ilya-biryukov added a comment to D50446: [clangd] Record the file being processed in a TUScheduler thread in context..

Short summary of the offline discussion: I suggest adding this parameter into the corresponding requests of the index (FuzzyFindRequest, FindDefinitionsRequest) instead of stashing it in the context. Context has all the same problems as the global variables, so we should try to avoid using it where possible.
On the other hand, where this key is stored might not be terribly important if we don't have too many usages and remove it when we can workaround the limitations that we're currently facing.

Wed, Aug 8, 6:54 AM
ilya-biryukov added a comment to D50443: [clang] Store code completion token range in preprocessor..

Maybe split the commit message into multiple lines?

Wed, Aug 8, 6:16 AM
ilya-biryukov added a comment to D50154: [clangd] capitalize diagnostic messages.

What's the motivation for clangd to differ from clang here?

The presentation of diagnostics to the users is different between clang and clangd:

  • clang diagnostics are always prefixed with the file, location and severity of the diagnostic, e.g. main.cpp:1:2: error: expected '}'
  • In LSP clients (VSCode in particular), the diagnostic message is the first thing the user sees and it looks a little nicer (subjectively) if the first letter is capitalized.
Wed, Aug 8, 5:00 AM
ilya-biryukov added a comment to D50415: [clangd] extend the publishDiagnostics response to send back fixits to the client directly as well (if requested).

+1 Sam's suggestion of configuring this during initial LSP handshake, rather than as a command-line flag.
We could put it into ClientCapabilities or into initializationOptions. The latter has type 'any' in LSP, so it seems to be most in-line with the protocol to put clangd-specific config options there, but not opposed to having this in other structs as an extension either.

Wed, Aug 8, 4:50 AM

Tue, Aug 7

ilya-biryukov accepted D50193: Added functionality to suggest FixIts for conversion of '->' to '.' and vice versa..

LGTM, with a few NITs.
Thanks for the change!

Tue, Aug 7, 10:18 AM
ilya-biryukov added a comment to D50193: Added functionality to suggest FixIts for conversion of '->' to '.' and vice versa..

Thanks! The only major comment I have is about the FixItsScore, others are mostly NITs.

Tue, Aug 7, 6:35 AM

Mon, Aug 6

ilya-biryukov accepted D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name.

This revision got 'reopened' and is now in the list of accepted revision. Should we close it again?

It got reverted a second time because it was breaking a test on Windows. The new bit is the change to test/PCH/case-insensitive-include.c, so it would need review again. If somebody else could run the tests on Windows, it would make me a bit more confident too.

Mon, Aug 6, 1:01 AM

Fri, Aug 3

ilya-biryukov added a comment to D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name.

This revision got 'reopened' and is now in the list of accepted revision. Should we close it again?

Fri, Aug 3, 9:10 AM
ilya-biryukov added inline comments to D50193: Added functionality to suggest FixIts for conversion of '->' to '.' and vice versa..
Fri, Aug 3, 9:05 AM
ilya-biryukov accepted D50189: Fully qualify the renamed symbol if the shortened name is ambiguous..

LGTM

Fri, Aug 3, 1:38 AM
ilya-biryukov added a comment to D50193: Added functionality to suggest FixIts for conversion of '->' to '.' and vice versa..

Thanks for the change! Overall LG, mostly NITs about the tests.

Fri, Aug 3, 1:32 AM

Thu, Aug 2

ilya-biryukov added inline comments to D50189: Fully qualify the renamed symbol if the shortened name is ambiguous..
Thu, Aug 2, 9:06 AM
ilya-biryukov added a comment to D50154: [clangd] capitalize diagnostic messages.

Seems we have consensus here.
Let's remove the option and just always capitalize the messages.

Thu, Aug 2, 7:10 AM
ilya-biryukov updated subscribers of D50154: [clangd] capitalize diagnostic messages.

Maybe we could simply always capitalize the messages? Any cons to capitalizing error messages?
@simark, @malaperle, @ioeric, @hokein, WDYT?

Thu, Aug 2, 4:43 AM

Wed, Aug 1

ilya-biryukov added a comment to D49658: [clangd] Index Interfaces for Xrefs.

Picking it up since @sammccall is OOO.

Wed, Aug 1, 6:59 AM
ilya-biryukov abandoned D50080: [Support] Use atomics in DebugCounter to silence TSAN.

Abandoning in favor of George's fix.

Wed, Aug 1, 2:28 AM
ilya-biryukov accepted D49833: [clangd] Receive compilationDatabasePath in 'initialize' request.

LGTM with a small nit. Thanks for the change!

Wed, Aug 1, 2:23 AM
ilya-biryukov accepted D49758: [clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request.

Thanks! LGTM

Wed, Aug 1, 2:20 AM · Restricted Project

Tue, Jul 31

ilya-biryukov added a comment to D50080: [Support] Use atomics in DebugCounter to silence TSAN.

This change is a bit ugly since it has to account for the value of LLVM_ENABLE_THREADS...
Happy to accept suggestions/alternative ways to fix this :-)

Tue, Jul 31, 9:59 AM
ilya-biryukov created D50080: [Support] Use atomics in DebugCounter to silence TSAN.
Tue, Jul 31, 9:57 AM
ilya-biryukov added a comment to D49794: [libclang] Allow skipping warnings from all included files.

Clang tidy is not only a standalone tool but also a plugin. It's almost never used this way (but we do that in Qt Creator to combine it with Clazy) and it also seems that some of the recent checks are added only to standalone tool (which is easy to fix).
Being loaded into clang as plugin it does not support most of clang-tidy command line options and is not able to filter system includes. I'm not sure it's possible to fix both of these issues (and probably some other).

Tue, Jul 31, 7:41 AM
ilya-biryukov added a comment to D49794: [libclang] Allow skipping warnings from all included files.

And we already saw, that -isystem is not the best choice for that.

Are you referring to the file-locking on Windows?
Any other reasons why the -isystem trick might be non-ideal?

Tue, Jul 31, 6:34 AM
ilya-biryukov updated the diff for D49991: [clangd] Do not build AST if no diagnostics were requested.
  • Rebase onto head
Tue, Jul 31, 6:12 AM
ilya-biryukov added a comment to D49794: [libclang] Allow skipping warnings from all included files.

I already mentioned in the review inherited by this one that this way covers also loaded plugins with different consumers. So let's assume that driver loads clang-tidy and some other plugins and runs over each file in the project.
It makes sense not to include clang-tidy diagnostics coming from headers, at least for third-party headers (you can still run clang over specific headers themselves to get diagnostics for them).

Tue, Jul 31, 6:06 AM
ilya-biryukov updated the diff for D50045: [clangd] Report diagnostics even if WantDiags::No AST was reused.
  • Check for PrevDiagsWereReported
Tue, Jul 31, 4:46 AM
ilya-biryukov added inline comments to D50045: [clangd] Report diagnostics even if WantDiags::No AST was reused.
Tue, Jul 31, 4:46 AM
ilya-biryukov added inline comments to D50045: [clangd] Report diagnostics even if WantDiags::No AST was reused.
Tue, Jul 31, 4:41 AM
ilya-biryukov updated the diff for D50045: [clangd] Report diagnostics even if WantDiags::No AST was reused.
  • Moved early return into the CanReuseAST branch
Tue, Jul 31, 4:39 AM
ilya-biryukov added inline comments to D49794: [libclang] Allow skipping warnings from all included files.
Tue, Jul 31, 4:38 AM
ilya-biryukov added inline comments to D50045: [clangd] Report diagnostics even if WantDiags::No AST was reused.
Tue, Jul 31, 3:35 AM
ilya-biryukov updated the diff for D50045: [clangd] Report diagnostics even if WantDiags::No AST was reused.
  • Added a comment
Tue, Jul 31, 3:35 AM
ilya-biryukov created D50045: [clangd] Report diagnostics even if WantDiags::No AST was reused.
Tue, Jul 31, 3:06 AM
ilya-biryukov added a comment to D49833: [clangd] Receive compilationDatabasePath in 'initialize' request.

Was there any objection to this patch? It would have been nice to have this before the branching.

Sorry for the delay, somehow missed this update in my inbox.

Tue, Jul 31, 1:36 AM
ilya-biryukov added a comment to D49758: [clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request.

Just a few nits left.

Tue, Jul 31, 1:23 AM · Restricted Project

Mon, Jul 30

ilya-biryukov created D49991: [clangd] Do not build AST if no diagnostics were requested.
Mon, Jul 30, 8:40 AM
ilya-biryukov added a comment to D49862: [clang-tidy] Fix a crash in fuchsia-multiple-inheritance.

Sorry for sending this in instead of waiting for D49158 to land, I hadn't noticed it earlier

Mon, Jul 30, 12:56 AM
ilya-biryukov added inline comments to D49758: [clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request.
Mon, Jul 30, 12:46 AM · Restricted Project

Fri, Jul 27

ilya-biryukov updated the diff for D49862: [clang-tidy] Fix a crash in fuchsia-multiple-inheritance.
  • Move the test into an existing file
Fri, Jul 27, 6:53 AM
ilya-biryukov added inline comments to D49758: [clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request.
Fri, Jul 27, 6:32 AM · Restricted Project
ilya-biryukov added a comment to D49758: [clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request.

The mode of operation where compile commands come from the client seems useful, but I wonder if there's any value in mixing it with compile_commands.json and other CDB plugins.
Do you plan to use the overridden commands in conjunction with CDB plugins or do you want the client to exclusively control the compile commands?

The client will control the commands exclusively.

Fri, Jul 27, 5:31 AM · Restricted Project

Thu, Jul 26

ilya-biryukov created D49862: [clang-tidy] Fix a crash in fuchsia-multiple-inheritance.
Thu, Jul 26, 9:15 AM
ilya-biryukov accepted D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name.

LGTM

Thu, Jul 26, 9:02 AM
ilya-biryukov added a comment to D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name.

Many thanks! Great cleanup. Just a few nits and we're good to go

Thu, Jul 26, 3:29 AM
ilya-biryukov added a comment to D49783: [clangd] Do not rebuild AST if inputs have not changed.

Thanks, that's simple and efficient. I'll update D49267 (to call reparseOpenFiles once again) once this is merged.

Thu, Jul 26, 2:22 AM
ilya-biryukov added inline comments to D49783: [clangd] Do not rebuild AST if inputs have not changed.
Thu, Jul 26, 2:21 AM
ilya-biryukov updated the diff for D49783: [clangd] Do not rebuild AST if inputs have not changed.
  • Move OldPreamble.reset() out of the lock, add a comment
Thu, Jul 26, 2:17 AM
ilya-biryukov added a comment to D49833: [clangd] Receive compilationDatabasePath in 'initialize' request.

Not strictly opposed to this change, but should any reason why the clients can't guarantee they'll send didChangeConfiguration right after clangd is initialized?

Thu, Jul 26, 2:05 AM

Wed, Jul 25

ilya-biryukov added inline comments to D49783: [clangd] Do not rebuild AST if inputs have not changed.
Wed, Jul 25, 7:57 AM
ilya-biryukov updated the diff for D49783: [clangd] Do not rebuild AST if inputs have not changed.
  • Use log instead of vlog
  • Add parens around &&
Wed, Jul 25, 7:55 AM
ilya-biryukov added a reviewer for D49267: [clangd] Watch for changes in compile_commands.json: ilya-biryukov.
Wed, Jul 25, 6:02 AM
ilya-biryukov accepted D49780: [clangd] Use a sigmoid style function for #usages boost in symbol quality..

LGTM

Wed, Jul 25, 3:51 AM
ilya-biryukov created D49783: [clangd] Do not rebuild AST if inputs have not changed.
Wed, Jul 25, 3:49 AM
ilya-biryukov added a comment to D49780: [clangd] Use a sigmoid style function for #usages boost in symbol quality..

Overall LG. I'm sure it's an improvement overall, just wanted to get some clarifying comments and references, if that's possible.

Wed, Jul 25, 2:16 AM
ilya-biryukov added a comment to D49724: [VFS] Cleanups to VFS interfaces..

For the record: this got reverted in rL337850

Wed, Jul 25, 1:30 AM

Tue, Jul 24

ilya-biryukov added a comment to D49758: [clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request.

The mode of operation where compile commands come from the client seems useful, but I wonder if there's any value in mixing it with compile_commands.json and other CDB plugins.
Do you plan to use the overridden commands in conjunction with CDB plugins or do you want the client to exclusively control the compile commands?

Tue, Jul 24, 11:54 PM · Restricted Project
ilya-biryukov added a comment to D49523: [clangd] Add support for per-file override compilation command.

We actually need both mechanisms. I posted the didChangeConfiguration extension to https://reviews.llvm.org/D49758.

Tue, Jul 24, 11:27 PM · Restricted Project
ilya-biryukov added a comment to D49267: [clangd] Watch for changes in compile_commands.json.

if/when we have a native implementation, supporting multiple mechanisms with different layering requirements to get at most a 2x win in watcher resource usage seems like a dubious way to spend our complexity budget.

I guess the only think that might force us to do this is the low system limit on the number of file watches. I do remember rumors that it can be a problem in, but I haven't really seen those on my own, so maybe I'm just pretending.
But I do agree that we shouldn't commit to have yet-another implementation in the long-term, unless we have good justification for it.

Tue, Jul 24, 9:51 AM
ilya-biryukov added a comment to D49267: [clangd] Watch for changes in compile_commands.json.

Ok, I agree that having clangd watch files itself could be necessary at some point (when the client does not support it), but it would have to be configurable. In our case, we have efficient enough file watching in the client, and already watch the files in the workspace. Since the inotify watches are limited per-user, having clangd watch them too means we'll have duplicates, and therefore waste a rather limited resource.

Actually, clangd could simply use the client capabilities. If the client advertises support for file watching with dynamic registration, use that, otherwise use the internal mechanism.

Yeah, sounds reasonable. The limits on the number of watches is definitely something that could bite the editor+clangd bundle, so that's a good reason to use the client watching if that's available.
We want to share some design around this area in Aug-Sep.

Tue, Jul 24, 6:40 AM
ilya-biryukov added a comment to D49267: [clangd] Watch for changes in compile_commands.json.

I guess you mean language client here, not language server. In our case we already have a client capable of file watching, so it's convenient for us to do that (plus, this is what the LSP specification recommends). If you want to make clangd capable of watching files natively, I am not against it, but it's not on our critical path. As long as the file is watched, I'm happy.

Yes, language client, thanks!

Tue, Jul 24, 4:46 AM
ilya-biryukov accepted D49719: [ClangFormat] Editor integrations inherit default style from clang-format binary.

LGTM

Tue, Jul 24, 2:26 AM
ilya-biryukov added inline comments to D49667: [clangd] Tune down quality score for class constructors so that it's ranked after class types..
Tue, Jul 24, 1:30 AM
ilya-biryukov added inline comments to D49719: [ClangFormat] Editor integrations inherit default style from clang-format binary.
Tue, Jul 24, 12:53 AM
ilya-biryukov accepted D49712: [docker] Fix LLVM_EXTERNAL_PROJECTS cmake variable value.

Wow, that's a huge oversight on my end.
Thanks for fixing this! LGTM

Tue, Jul 24, 12:28 AM