ioeric added inline comments.
================
Comment at: clangd/CodeComplete.cpp:457
Result.StartsNestedNameSpecifier = false;
+ // FIXME: the same result can be added multiple times as the callback can
+ // be called more than once. We may want to deduplicate identical
results.
----------------
ilya-biryukov wrote:
> Could we fix this in the first attempt?
> All the code around completion is really not prepared for multiple callbacks:
> index will be queried multiple times, we will store and log only the last
> CCContext, not all of them, etc.
> I'd argue we should aim to either provide a single-callback completion or
> rewrite the whole code around completion to properly handle multiple
> callbacks (i.e. with deduplication and proper merging of the results coming
> from multiple callbacks, proper logging, no multiple identical requests to
> the index).
>
> I would suggest the following measures as a hacky intermediate solution:
> - Ignore natural language completion. The rationale: VSCode does analogous
> completion on empty results anyway, AFAIK clang does not provide any useful
> results on top of that. Other clients that we have can (and should?) probably
> do the same.
> - Only record the first non-natural language completion attempt. Ignore the
> rest and log the failed attempts.
> index will be queried multiple times
Patterns of multi-context callbacks I've seen are:
1) natural language + Name: this happens mostly when parsing macros with
stringification.
2) name + name: this happens in the pre-existing unit test case. I don't really
understand why and how often this comes up, but I think the duplication should
be eliminated.
3) language+recovery: haven't looked into what the recovery context does.
> we will store and log only the last CCContext
What's the concern about storing only the last context?
> I'd argue we should aim to either provide a single-callback completion or
> rewrite the whole code around completion to properly handle multiple
> callbacks (
I'm not sure how we could (fully) get away with one callback without
significantly changing sema parsing. This seems to be an expensive approach
though.
>with deduplication and proper merging of the results coming from multiple
>callbacks
I agree. My impression is that multiple callbacks are not common and thus not
as important (duplicates are better than no results IMO). But I might be wrong
thinking this is uncommon. I was going to do this in a followup patch to avoid
a big patch, but I'm happy to do the deduplication in the same patch if you
prefer.
> proper logging
Could you point out what logging is missing?
> no multiple identical requests to the index
For context combinations I've seen (natural language + name, natural language +
recovery), index is still queries once. If sema does decide to call name
multiple times with context that would potentially yield two index queries, we
could still need to query indexes twice (don't see a big problems doing this if
not a common case). For identical context that is called multiple times, we
could cache potentially results.
> I would suggest the following measures as a hacky intermediate solution:
I think natural language is only one of the contexts that could result in
multiple callbacks, so I don't think this would fully resolve our problems.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47183
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
