ilya-biryukov (Ilya Biryukov)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Tue, Oct 16

ilya-biryukov added a comment to D53192: [clangd] Do not query index for new name completions..

A drive-by NIT ;-)

Tue, Oct 16, 4:29 PM
ilya-biryukov created D53347: [clangd] Simplify auto hover.
Tue, Oct 16, 4:16 PM

Thu, Oct 11

ilya-biryukov accepted D53135: Remove top-level using declaration from header files, as these aliases leak..

LGTM

Thu, Oct 11, 8:06 AM
ilya-biryukov added a comment to D52311: [clangd] Add support for hierarchical documentSymbol.

I just tried this, this looks very promising! It should help build our outline view in a much more robust way than we do currently.
A nit for the final patch, I would suggest omitting the fields that are optional, like children (when the list is empty) and deprecated.

SG, will do.

Thu, Oct 11, 5:38 AM
ilya-biryukov updated subscribers of D51725: Allow un-setting the compilation database path.

But I am wondering what to do with the feature that allows clangd to change compilation database path using the didChangeConfiguration notification. In my opinion, it's a bug that it's not possible to switch back to use no explicit compilation database path, so I'd still like to get this patch merged. Or, if we decide this is really not useful, then it should be removed. I don't think we should keep the feature in the current state, either we fix it or remove it.

Fair point. The original patch looked scary mostly because of Optional<Optional<...>>. Many thanks for clearing up your use-case, though. I thought Theia was exclusively using custom directories for compile_commands.json, I didn't know the user had an option to turn this behavior on/off and configure directories explicitly.
I would question the usefulness of the option if Theia can live without it, so I'd vouch for removing it. @sammccall, WDYT? Especially wrt to auto-indexing and it being a customizable build directory?

Thu, Oct 11, 5:35 AM

Thu, Oct 4

ilya-biryukov accepted D52796: [clangd] Simplify Dex query tree logic and fix missing-posting-list bug.

Putting a stamp, assuming is answer is "yes, it's a legacy way to specify all scopes"

Thu, Oct 4, 8:53 AM
ilya-biryukov added a comment to D52796: [clangd] Simplify Dex query tree logic and fix missing-posting-list bug.

Overall LGTM, just a quick question to make sure I get what's going on.

Thu, Oct 4, 8:48 AM
ilya-biryukov added inline comments to D52872: [clangd] Make binary index format the default, remove dead flag..
Thu, Oct 4, 3:37 AM
ilya-biryukov accepted D52789: [clangd] Dex: FALSE iterator, peephole optimizations, fix AND bug.

LGTM and a few NITs.

Thu, Oct 4, 3:05 AM

Tue, Oct 2

ilya-biryukov accepted D52647: [CodeComplete] Re-fix accessibilty of protected members from base class..

LGTM

Tue, Oct 2, 3:06 AM

Mon, Oct 1

ilya-biryukov added a comment to D52710: [clangd] Add "check-clangd" target.

This is awesome! Can't wait for it to land!

Mon, Oct 1, 5:44 AM
ilya-biryukov added a comment to D52620: Added Support for StatOnly Files in VFS..

I would call it a requirement instead of an assumption. The replay must be exactly the same (even the file stats and reads). If Clang reads the file in replay which was only stat()ed during compilation, it indicates non-determinism or something wrong (in clang or FS).
We currently deal with such files by adding empty buffers for them based on this assumption/requirement only.

Mon, Oct 1, 5:21 AM

Wed, Sep 26

ilya-biryukov added a comment to D52547: Tell whether file/folder for include completions..

A drive-by comment.
Would it be cleaner to pass this information from clang? Relying on completion label seems shaky.

Wed, Sep 26, 8:21 AM

Tue, Sep 25

ilya-biryukov added inline comments to D52419: [clangd] Cache FS stat() calls when building preamble..
Tue, Sep 25, 11:05 PM
ilya-biryukov added inline comments to D52422: [clangd] Handle template args for disabled function arg snippets.
Tue, Sep 25, 10:43 PM
ilya-biryukov updated the diff for D52422: [clangd] Handle template args for disabled function arg snippets.
  • Remove debug formatting
  • Update snippet for no-arg case with the suggested changes
Tue, Sep 25, 10:43 PM

Mon, Sep 24

ilya-biryukov added inline comments to D52419: [clangd] Cache FS stat() calls when building preamble..
Mon, Sep 24, 10:04 PM
ilya-biryukov added inline comments to D52273: [clangd] Initial implementation of expected types.
Mon, Sep 24, 10:03 AM
ilya-biryukov added a comment to D52273: [clangd] Initial implementation of expected types.

This seems very clever, but extremely complicated - you've implemented much of C++'s conversion logic, it's not clear to me which parts are actually necessary to completion quality.

Clearly the model that supports C++ conversions is something that will improve code completion quality.
I do agree it's not trivial, but would argue we at least want:

  • qualification conversions (i.e. adding const)
  • user-defined conversions (e.g. operator bool is commonly useful think)
  • derived-to-base conversions (Derived* should convert to Base*)
Mon, Sep 24, 9:56 AM
ilya-biryukov added inline comments to D52422: [clangd] Handle template args for disabled function arg snippets.
Mon, Sep 24, 9:42 AM
ilya-biryukov added inline comments to D52420: [clangd] Fix crash if pending computations were active on exit.
Mon, Sep 24, 9:42 AM
ilya-biryukov accepted D50171: [python] [tests] Update test_code_completion.

@ilya-biryukov, gentle ping. I'd like to patch this for 7.0.0 in Gentoo. Do you think my patch would be good enough, or do you expect to submit something else soonish?

Mon, Sep 24, 9:04 AM
ilya-biryukov created D52422: [clangd] Handle template args for disabled function arg snippets.
Mon, Sep 24, 7:59 AM
ilya-biryukov created D52420: [clangd] Fix crash if pending computations were active on exit.
Mon, Sep 24, 7:39 AM

Fri, Sep 21

ilya-biryukov accepted D52261: [Sema] Generate completion fix-its for C code as well.

Thanks! LGTM!

Fri, Sep 21, 4:07 AM

Thu, Sep 20

ilya-biryukov planned changes to D52311: [clangd] Add support for hierarchical documentSymbol.

Posted to make sure it's visible that I've started doing this.
Still need to update the tests and check for the capability from the client (and fallback to SymbolInformation if client does not support the new implementation).

Thu, Sep 20, 9:17 AM
ilya-biryukov created D52311: [clangd] Add support for hierarchical documentSymbol.
Thu, Sep 20, 9:11 AM
ilya-biryukov added a comment to D52261: [Sema] Generate completion fix-its for C code as well.

I tried that first but did not I find a way just to copy an expression (we basically need the same expr for such case). Do you know how to properly generate a copy of expression or some other way to get the same expression?

Thu, Sep 20, 6:49 AM
ilya-biryukov added a comment to D52047: [clangd] Add building benchmark and memory consumption tracking.

Also not sure about the trick:

  • Would be surprising to see the "ms" instead of "mbytes"
  • Time tends to vary between runs, so using Google Benchmark's capabilities to run multiple times and report aggregated times seems useful. Not so much for the memory usage: it's deterministic and running multiple times is just a waste of time.
Thu, Sep 20, 6:37 AM · Restricted Project

Wed, Sep 19

ilya-biryukov added a comment to D52261: [Sema] Generate completion fix-its for C code as well.

It's the responsibility of the caller to provide a corrected expression, this keeps the completion function simpler. I suggest we stick to this contract.
We do something in getLangOpts().CPlusPlus case to create a corrected expression, let's just implement a similar fallback for C at the single callsite that we have (ParsePostfixExpressionSuffix).

Wed, Sep 19, 12:28 PM
ilya-biryukov planned changes to D52276: [clangd] Add type boosting in code completion.

Ranking-related code should be moved to Quality.cpp

Wed, Sep 19, 12:19 PM
ilya-biryukov added a dependent revision for D52274: [clangd] Collect and store expected types in the index: D52276: [clangd] Add type boosting in code completion.
Wed, Sep 19, 12:19 PM
ilya-biryukov created D52276: [clangd] Add type boosting in code completion.
Wed, Sep 19, 12:19 PM
ilya-biryukov added a dependency for D52276: [clangd] Add type boosting in code completion: D52274: [clangd] Collect and store expected types in the index.
Wed, Sep 19, 12:19 PM
ilya-biryukov added a dependency for D52273: [clangd] Initial implementation of expected types: D52275: [Index] Expose USR generation for types.
Wed, Sep 19, 12:17 PM
ilya-biryukov added a dependent revision for D52275: [Index] Expose USR generation for types: D52273: [clangd] Initial implementation of expected types.
Wed, Sep 19, 12:17 PM
ilya-biryukov created D52275: [Index] Expose USR generation for types.
Wed, Sep 19, 12:16 PM
ilya-biryukov planned changes to D52274: [clangd] Collect and store expected types in the index.

Need to store the types in RIFF format too

Wed, Sep 19, 12:16 PM
ilya-biryukov added a dependent revision for D52273: [clangd] Initial implementation of expected types: D52274: [clangd] Collect and store expected types in the index.
Wed, Sep 19, 12:15 PM
ilya-biryukov added a dependency for D52274: [clangd] Collect and store expected types in the index: D52273: [clangd] Initial implementation of expected types.
Wed, Sep 19, 12:15 PM
ilya-biryukov created D52274: [clangd] Collect and store expected types in the index.
Wed, Sep 19, 12:14 PM
ilya-biryukov added inline comments to D52273: [clangd] Initial implementation of expected types.
Wed, Sep 19, 12:13 PM
ilya-biryukov added a comment to D52273: [clangd] Initial implementation of expected types.

The implementation might look a bit scary, please feel free to ask for comments/clarifications!

Wed, Sep 19, 12:08 PM
ilya-biryukov created D52273: [clangd] Initial implementation of expected types.
Wed, Sep 19, 12:07 PM

Sep 19 2018

ilya-biryukov accepted D51214: [clangd] Add option to enable/disable function argument snippets..

LGTM

Sep 19 2018, 2:08 AM
ilya-biryukov accepted D52079: [Sema] Do not load macros from preamble when LoadExternal is false..

LGTM

Sep 19 2018, 2:03 AM

Sep 18 2018

ilya-biryukov added a comment to D51214: [clangd] Add option to enable/disable function argument snippets..

Maybe commit only an option to enable function arg snippets for now (found myself wanting this option today :-))? The fixes would also be nice, but since they never work...

Sep 18 2018, 12:39 AM
ilya-biryukov accepted D52076: [CodeComplete] Add completions for filenames in #include directives..

LGTM

Sep 18 2018, 12:27 AM
ilya-biryukov added a comment to D52079: [Sema] Do not load macros from preamble when LoadExternal is false..

Maybe add a test?
There is test/Index/complete-pch-skip.cpp that checks similar things for AST completions, adding macros there should be trivial.

Sep 18 2018, 12:20 AM
ilya-biryukov accepted D52098: [Index] Add an option to collect macros from preprocesor..

LGTM

Sep 18 2018, 12:13 AM

Sep 17 2018

ilya-biryukov added inline comments to D52076: [CodeComplete] Add completions for filenames in #include directives..
Sep 17 2018, 8:14 AM
ilya-biryukov added a comment to D52083: [clangd] Store OR iterator children in heap.

Seems like this optimization is not worth (yet?). As soon as we get more proximity paths (and hence more OR iterator children) that might make sense.

WDYT about storing all the elements with the minimal doc-id outside the heap? I.e. we can pop all elements with the minimum doc-id on 'advance' and iterator creation.
That way we could potentially regain the improvements we've seen in the first version.

Sep 17 2018, 5:16 AM · Restricted Project
ilya-biryukov added a comment to D52098: [Index] Add an option to collect macros from preprocesor..

Overall LG, see the major comment about running without the preprocessor.

Sep 17 2018, 3:59 AM
ilya-biryukov added a comment to D52076: [CodeComplete] Add completions for filenames in #include directives..

Sorry for the late response.

Sep 17 2018, 3:40 AM
ilya-biryukov added a comment to D45719: [clang-Format] Fix indentation of member call after block.

Landed the patch as r342363.

Sep 17 2018, 12:50 AM

Sep 14 2018

ilya-biryukov added inline comments to D52078: [clangd] Store preamble macros in dynamic index..
Sep 14 2018, 5:00 AM
ilya-biryukov added inline comments to D52083: [clangd] Store OR iterator children in heap.
Sep 14 2018, 4:05 AM · Restricted Project
ilya-biryukov added a comment to D52078: [clangd] Store preamble macros in dynamic index..

~1% increase in memory usage seems totally fine. Actually surprised it's that small.
Overall LG, just a single comment about extra copying. Thanks for the change, looks like a great improvement!

Sep 14 2018, 4:01 AM
ilya-biryukov added inline comments to D52083: [clangd] Store OR iterator children in heap.
Sep 14 2018, 3:49 AM · Restricted Project
ilya-biryukov added a comment to D52076: [CodeComplete] Add completions for filenames in #include directives..

Amazing, can't wait for it to land!

Sep 14 2018, 3:33 AM
ilya-biryukov accepted D52071: [clangd] Don't override the preamble while completing inside it, it doesn't work..

Neat, I was unaware that single-file mode completions actually work nicely in that case.
LGTM!

Sep 14 2018, 2:29 AM

Sep 13 2018

ilya-biryukov added a comment to D51747: [clangd] Show deprecation diagnostics.

Np, not planning to land it before we figure out the issue with vim can't handling lots of diagnostics anyway.

Sep 13 2018, 8:50 AM
ilya-biryukov accepted D51971: [clangd] Use JSON format in benchmark requests reader.

LGTM

Sep 13 2018, 7:18 AM · Restricted Project
ilya-biryukov added a comment to D51725: Allow un-setting the compilation database path.

I am not sure I understand correctly your last point. Of course, when restarting clangd, we need to send again a didOpen for each file the user has currently open in the editor. But that should be it?

Yes, this and any initial configuration settings.
My personal experience is from VSCode, which does not call addDocument IIUC. This results in clangd producing errors about running actions in non-opened files.

Sep 13 2018, 7:03 AM
ilya-biryukov added inline comments to D51971: [clangd] Use JSON format in benchmark requests reader.
Sep 13 2018, 6:43 AM · Restricted Project
ilya-biryukov accepted D51297: [docs] Provide pointers to known editor plugins and extensions.

LGTM

Sep 13 2018, 3:18 AM · Restricted Project
ilya-biryukov added inline comments to D51971: [clangd] Use JSON format in benchmark requests reader.
Sep 13 2018, 3:11 AM · Restricted Project
ilya-biryukov added inline comments to D51297: [docs] Provide pointers to known editor plugins and extensions.
Sep 13 2018, 3:00 AM · Restricted Project
ilya-biryukov added a comment to D52016: [clangd] Don't create child AND and OR iterators with one posting list.

Great improvement for such a simple change!

Sep 13 2018, 2:57 AM · Restricted Project
ilya-biryukov accepted D52016: [clangd] Don't create child AND and OR iterators with one posting list.

LGTM with a nit.

Sep 13 2018, 2:57 AM · Restricted Project
ilya-biryukov added inline comments to D52016: [clangd] Don't create child AND and OR iterators with one posting list.
Sep 13 2018, 1:17 AM · Restricted Project
ilya-biryukov added inline comments to D51971: [clangd] Use JSON format in benchmark requests reader.
Sep 13 2018, 1:17 AM · Restricted Project
ilya-biryukov added a comment to D51996: [clangd] Simplify cancellation public API.

Will let Kadir finish the review, consider lgtm'ed from me ;-)

Sep 13 2018, 12:20 AM
ilya-biryukov added a comment to D51996: [clangd] Simplify cancellation public API.

but the thing you're spending the CPU on is checking for cancellation...

Unless checking for cancellation is really cheap (which is doable, I think).
We should probably hit some of those in practice before doing something, though.

Sep 13 2018, 12:17 AM
ilya-biryukov added inline comments to D51971: [clangd] Use JSON format in benchmark requests reader.
Sep 13 2018, 12:08 AM · Restricted Project
ilya-biryukov added a comment to D51725: Allow un-setting the compilation database path.

If this setting exposed directly the users in Theia or is it something that is exposed via a custom UI (e.g. choosing named build configs or something similar)?

Sep 13 2018, 12:01 AM

Sep 12 2018

ilya-biryukov added a comment to D51989: [clangd] dexp tool uses llvm::cl to parse its flags..

A small drive-by comment.

Sep 12 2018, 9:20 AM
ilya-biryukov added a comment to D51971: [clangd] Use JSON format in benchmark requests reader.

Apparently, the parsing seems to be wrong; I should fix that first.

Sep 12 2018, 9:14 AM · Restricted Project
ilya-biryukov added a comment to D51725: Allow un-setting the compilation database path.

Wow, this is getting somewhat complicated.

Sep 12 2018, 9:13 AM
ilya-biryukov retitled D51987: [clangd] Rename global-symbol-builder to clangd-indexer. from [clangd] Rename global-symbol-builder to clangd-symbol-builder. to [clangd] Rename global-symbol-builder to clangd-indexer..
Sep 12 2018, 8:54 AM
ilya-biryukov added a comment to D51987: [clangd] Rename global-symbol-builder to clangd-indexer..

Thanks for the comments everyone!
Will land this tomorrow, since I'm buildcop this week anyway.

Sep 12 2018, 8:51 AM
ilya-biryukov added a comment to D51987: [clangd] Rename global-symbol-builder to clangd-indexer..

You beat it to me, but I thought we didn't use indexer as it could be confused with the actual indexing?

Sep 12 2018, 8:05 AM
ilya-biryukov updated the diff for D51987: [clangd] Rename global-symbol-builder to clangd-indexer..
  • Rename to clangd-indexer
Sep 12 2018, 7:59 AM
ilya-biryukov added a comment to D51987: [clangd] Rename global-symbol-builder to clangd-indexer..

clangd-indexer looks nice and short. @ioeric, WDYT?

Sep 12 2018, 7:52 AM
ilya-biryukov added inline comments to D51971: [clangd] Use JSON format in benchmark requests reader.
Sep 12 2018, 7:41 AM · Restricted Project
ilya-biryukov accepted D51977: [clangd] Clarify and hide -index flag..

+1 to making this hidden. Users (most of them) shouldn't care about it.
Is there any motivation on why we someone would want to disable it currently? Maybe we want to deprecate it and remove it completely at some point?
I could only think of being as close as possible to sema completions, but that doesn't seem like something that gives better UX (folks from Apple might be interested in that, but not sure).

Sep 12 2018, 7:39 AM
ilya-biryukov added a comment to D51987: [clangd] Rename global-symbol-builder to clangd-indexer..

We've used the new name internally before and now that we're testing this, we need to be consistent.
The 'clangd-symbol-builder' looks like a better choice, so I'm pitching changing upstream first.

Sep 12 2018, 7:28 AM
ilya-biryukov updated the diff for D51987: [clangd] Rename global-symbol-builder to clangd-indexer..
  • Rebase, updated the added test
Sep 12 2018, 7:28 AM
ilya-biryukov created D51987: [clangd] Rename global-symbol-builder to clangd-indexer..
Sep 12 2018, 7:26 AM
ilya-biryukov added a comment to D50171: [python] [tests] Update test_code_completion.

Will take a closer look. Thanks for finding the offending revision.

Sep 12 2018, 12:02 AM

Sep 11 2018

ilya-biryukov added inline comments to D51628: [clangd] Implement a Proof-of-Concept tool for symbol index exploration.
Sep 11 2018, 8:43 AM · Restricted Project
ilya-biryukov added inline comments to D51628: [clangd] Implement a Proof-of-Concept tool for symbol index exploration.
Sep 11 2018, 7:46 AM · Restricted Project
ilya-biryukov accepted D51917: [CodeCompletion] Enable signature help when initializing class/struct/union members..

LGTM. Thanks for the fix!

Sep 11 2018, 7:40 AM
ilya-biryukov added inline comments to D51214: [clangd] Add option to enable/disable function argument snippets..
Sep 11 2018, 7:34 AM
ilya-biryukov added a comment to D51747: [clangd] Show deprecation diagnostics.

A few thoughts here:

  • does CDB describe user or project preferences? unclear.

Agree, it's a mix, defaults are from the project but users can add extra flags.

Sep 11 2018, 6:43 AM
ilya-biryukov added a comment to D51214: [clangd] Add option to enable/disable function argument snippets..

+1 to adding an option to drop arguments from snippets and removing the option for the fixes.

Sep 11 2018, 6:07 AM
ilya-biryukov added inline comments to D51917: [CodeCompletion] Enable signature help when initializing class/struct/union members..
Sep 11 2018, 5:23 AM
ilya-biryukov accepted D51924: [clangd] Add unittests for D51917.

LGTM

Sep 11 2018, 4:47 AM
ilya-biryukov added inline comments to D51917: [CodeCompletion] Enable signature help when initializing class/struct/union members..
Sep 11 2018, 4:46 AM
ilya-biryukov added inline comments to D51214: [clangd] Add option to enable/disable function argument snippets..
Sep 11 2018, 3:49 AM