- User Since
- Apr 6 2017, 1:42 AM (80 w, 1 d)
Tue, Oct 16
A drive-by NIT ;-)
Thu, Oct 11
SG, will do.
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 4
Putting a stamp, assuming is answer is "yes, it's a legacy way to specify all scopes"
Overall LGTM, just a quick question to make sure I get what's going on.
LGTM and a few NITs.
Tue, Oct 2
Mon, Oct 1
This is awesome! Can't wait for it to land!
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.
Wed, Sep 26
A drive-by comment.
Would it be cleaner to pass this information from clang? Relying on completion label seems shaky.
Tue, Sep 25
- Remove debug formatting
- Update snippet for no-arg case with the suggested changes
Mon, Sep 24
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*)
Fri, Sep 21
Thu, Sep 20
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).
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.
Wed, Sep 19
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).
Ranking-related code should be moved to Quality.cpp
Need to store the types in RIFF format too
The implementation might look a bit scary, please feel free to ask for comments/clarifications!
Sep 19 2018
Sep 18 2018
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...
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 17 2018
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.
Overall LG, see the major comment about running without the preprocessor.
Sorry for the late response.
Landed the patch as r342363.
Sep 14 2018
~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!
Amazing, can't wait for it to land!
Neat, I was unaware that single-file mode completions actually work nicely in that case.
Sep 13 2018
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.
Great improvement for such a simple change!
LGTM with a nit.
Will let Kadir finish the review, consider lgtm'ed from me ;-)
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.
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 12 2018
A small drive-by comment.
Wow, this is getting somewhat complicated.
Thanks for the comments everyone!
Will land this tomorrow, since I'm buildcop this week anyway.
You beat it to me, but I thought we didn't use indexer as it could be confused with the actual indexing?
- Rename to clangd-indexer
clangd-indexer looks nice and short. @ioeric, WDYT?
+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).
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.
- Rebase, updated the added test
Will take a closer look. Thanks for finding the offending revision.
Sep 11 2018
LGTM. Thanks for the fix!
Agree, it's a mix, defaults are from the project but users can add extra flags.
+1 to adding an option to drop arguments from snippets and removing the option for the fixes.