o Lex the code to get the identifiers and put them into a "symbol" index.
o Adds a new completion mode without compilation/sema into code completion workflow.
o Make IncludeInserter work even when no compile command is present, by avoiding
inserting non-verbatim headers.
Details
Diff Detail
- Repository
- rCTE Clang Tools Extra
- Build Status
Buildable 30362 Build 30361: arc lint + arc unit
Event Timeline
clangd/CodeComplete.cpp | ||
---|---|---|
1142 | as discussed offline, I love the lexer approach but returning it as an index seems like the wrong level of abstraction. Can we move it to SourceCode.h too? | |
1142 | I don't think we need FileName as a parameter - it shouldn't affect the return value, so we can just use a dummy value. | |
1189 | I'm not sure these changes (mostly to comment formatting) improve clarity. | |
1192 | ReplacedRange? | |
1289 | runWithoutSema maybe? - compile is a fine name but this file calls this concept sema for whatever reason | |
1291 | hmm, this function should likely also move to SourceCode, use Offset instead of Position, and move to SourceCode.h. But probably a different patch. | |
1312 | you could just erase the typed filter from the suggestion list. | |
1328 | I suspect you want to pass the filtered strings through to mergeResults and have it/CompletionCandidate do the scoring. | |
clangd/CodeComplete.h | ||
248 | It seems a little odd to have a different entrypoint here. It seems it might even make sense to use the same speculative fuzzyfind? |
Thanks for the review!
clangd/CodeComplete.cpp | ||
---|---|---|
1291 | ack | |
1312 | This is following conventions of other sources. Both index and sema provide results for fully-typed names. Considering that users might be already used to this, and completion results tend to give reassurance for correctly typed names, I am inclined to keep the typed filter if it's seen somewhere else in the file. |
Very nice! D60503 will conflict, feel free to stall that until this is landed.
On the other hand it will simplify some things, e.g. the prefix is already calculated, and typed scope is available if you want that (no enclosing namespaces though).
clangd/CodeComplete.cpp | ||
---|---|---|
185 | This is a bit vague - most results are for identifiers in some sense. | |
281 | move this comment to the member? | |
1178 | rename NBoth -> NSemaAndIndex? | |
1306 | it doesn't make sense to set this here, as long as we're not querying the index. | |
1312 | OK, but we should now be able to just decrement the count for the right entry in the collected identifiers then? rather than carving up the string | |
1355 | add this to the non-sema case too (though the if is always false for now), or add a fixme? | |
1516 | why are we not initializing fileproximity in the fallback case? | |
1558 | can we have a SymbolOrigin bit for identifiers, and use that instead? | |
clangd/Headers.cpp | ||
182 | nit: i'd move this above the previous case, it seems both more fundamental and cheaper | |
196 | Do we expect this code path to ever be called? | |
clangd/SourceCode.cpp | ||
403 | why isn't Add just ++Identifiers[Id]? |
- address review comments
clangd/CodeComplete.cpp | ||
---|---|---|
1355 | Added a FIXME. | |
clangd/Headers.cpp | ||
196 | This can be hit when we start serving index symbols in fallback mode completion. shouldInsertInclude will return false, but the calculation will be attempted regardless. We can probably elog this, but the number of log messages would be O(# of index symbols), which I worry might be too spammy. |
It seems a little odd to have a different entrypoint here.
Could we simply indicate this with Preamble=nullptr?
It seems it might even make sense to use the same speculative fuzzyfind?
So I think PCHContainerOperations would be the only odd one out. I'm going to send a patch to remove that anyway :-)