- User Since
- Apr 6 2017, 1:42 AM (59 w, 9 h)
Added the test.
- Added a test for preamble rebuilds.
- Invert flag, remove the default.
I have only a few nits left, otherwise looks good. Thanks for making this change! Really excited for it to get in finally!
But I feel it's a bit odd that completion and workspace symbols would be inconsistent.
It does not seem that odd to me. Completion is something that should follow the language rules more closely, i.e. we don't want to deviate from sema completions too much. While workspaceSymbol is a user-facing feature and we probably want users to type as little as possible there.
E.g. not showing namespace_name::A in completion seems bad to me, since people are used to referring to the enumerators this way and sema completion gives you those results.
- Simplify the change by boosting any Decls that have a redecl in the current file
After an offline chat: we decided to boost sema results that have any decls in the main file. Even though it introduces some unwanted inconsistencies in some cases (e.g. see the comment), most of us agree that's a very simple implementation that does boost useful things, including stuff from association header.
To get better results, we need to design and build the real proximity scoring that handles associated headers and other things.
- Reimplemented LRU cache with shared_ptr and weak_ptr.
- Added forgotten renames
@malaperle, a slight offtopic. I was wondering which completion options do you use for clangd? Defaults?
Yeah. I really wish we had microbenchmarks for things like completion.
I'm not sure if we have tests for that, but I remember that we kept the enumerators in the outer scope so that completion could find them..
Am I right that this patch will change the behavior and we won't get enumerators in the following example:
Tue, May 22
I've added an initial version of testing for the matching header and wanted to get feedback before proceeding further with tests and other changes.
- Move helpers tha switch header and source into separate files.
- Also uprank items coming from the matching headers
Fri, May 18
LGTM with a minor naming suggestion.
Another alternative that I've considered was evicting the ASTs from memory after a certain period of time, e.g. after 30 seconds of inactivity for some file. This might be simpler and also cover the use-case of speeding up multiple code navigation requests (findDefinition/documentHighlight) in a row.
Yet another thing that came to mind: walking over all of the AST to find reference under the cursor is terribly inefficient, we should be able to build a cheap data structure that speeds up this operation. Maybe we can even store enough information to not need the AST for navigation at all and build it only for features like refactorings.
@sammccall, let me know what are your thoughts on all of this.
This certainly does not have to be addressed in this patch. Just wanted to collect opinions on what the behavior we want in the long term.
My thoughts would be towards allowing only vector::push_back and make it match both push_backs: in vector<bool> and vector<T>.
Other cases might work too, but I wouldn't try implementing something that matches specializations. It's just too complicated in the general case. This will only be used in workspaceSymbol and it's fine to give a few more results there...
Thu, May 17
- Move tests to QualityTests.cpp
- Fix findDecl() assertion on redecls of the same thing
- Fix file comment of TestTU.cpp
A few questions regarding class members. To pinpoint some interesting cases and agree on how we want those to behave in the long run.
Sorry, a few more things and we're good to go.
Wed, May 16
Rebase onto head, fix merge conflicts
Tue, May 15
- Added tests that all comments are being parsed.
- Fix code after a change in deps (getFormattedText now needs a SourceManager instead of an ASTContext)
- Address review comments
- Rebase onto head and remove \brief from the comments, it's gone upstream now
- Rename CurrentArg to ArgIndex
Maybe move the tests back to this revision?
There are tests for code completion that don't depend on libindex or libclang and use clang -cc1 directly, i.e. tools/clang/test/CodeComplete. This would require adding a flag to clang and patching up PrintingCodeCompleConsumer to print the fix-its, but that should be easy. Moreover, it's useful to have the option to show those completions in clang anyway.
- Removed the overload that accepts ASTContext
Mon, May 14
This LG, presuming there are no semantic changes here, just moving the code around.
Also see the nits about IncludeStyle that seems to be in the wrong change...
LG. Just a quick comment about possibly unrelated change.
- Address review comments
Sat, May 12
Fri, May 11
Didn't take along to find an example. Thanks for digging this up. That looks like a good enough reason to not apply proto-specific filtering based solely on the filename...
- Simplify test code with SourceManagerForFile.
LG when all the dependencies are done
So, the first line of the file generated by proto compiler seems to be something like this:
// Generated by the protocol buffer compiler. DO NOT EDIT!
This change is big enough to be somewhat hard to grasp.
Maybe we could split it into two, to make it easier for review?
- Extraction of IncludeStyle into a separate subclass.
- Moving stuff around without other refactorings.
LGTM, just a few non-blocking NITs with questions.
This LG, but we should first roll out the additionalEdits change.
Aren't the dependencies reversed in the dependency stack, i.e. this change should be the last one?
Wed, May 9
- Moved tests to clang
- Renames and other comments
- Don't include brief comments in signature help either, comments there are also handled by the code completion code now.
Tue, May 8
- Fixed infinite loop with comments that contain doxygen commands
@arphaman, friendly ping. In case you're the wrong person to review it, I'll try to find someone else.
- Move unit tests from clangd code to AST tests
- Assert locations are valid
- Address review other comments
The new uploaded diff has lots of unrelated changes to clang-tidy, clang-move, etc...
Looking at commits, it seems arc diff was called with the wrong base commit...
Could you please reupload the change?
Mon, May 7
The change makes both testing and scoring code better.
Even though those are largely independent changes, perfectly happy to review them together.
Fri, May 4
I somehow missed the review email, sorry for the delayed response.
Sorry for the delay. I really like the direction of this patch!
What's left is defining the semantics of corrections more thoroughly to make sure we don't have tricky corner cases that users of the API can't deal with.
Sorry for the delay
Thu, May 3
Wed, May 2
I would say "yes". Let's not rely on linemarkers, unless we can explain why that's a good idea.
This is really a corner cases that users might not need to know about. But an example is:
Code: "#include <a>" (without \n at the end). After inserting <x>, #include <a>\n#include <x>\n (this is good). However, if you insert another <y>, the code would become "#include <a>\n#include <x>\n\n#include <y>\n" (note the double newline!).
Even though that's rare in practice, it seems like an actual bug.
How hard would be it be to fix it?
Fri, Apr 27
the patch LG and fixes the problem of llvm namespace not showing up in completions from the static index.
Let's add some tests and ship it!
This change LG as an extraction of the helper functionality to be reused in clang, clang-tidy, etc.
However, I feel there are potential improvements both to the underlying code and the new APIs that we could make.
Thu, Apr 26
I still don't get why the file is virtual. Looking at the code in SourceManager, presumed location uses FileEntry of passed location (actually, its expansion location, but that shouldn't matter) and then translates line numbers according to the #line directives in the file.
So the question is: why is FileEntry null for original location, but not null for PresumedLoc?
OK, I've rechecked this change. I don't see any obvious mistake :)
I think I got to the bottom of it. We didn't expect a big win, because we expect people to not put their non-template code into the header files. Which is probably true.
The problem with our reasoning, is that this patch also skip bodies of non-template functions inside template classes, e.g.
Could you upload the patch with full context?
- Remove tryLexCommands(), call into helper that parses commands directly
- Addressed other review comments