ioeric added a comment.
Thanks for the review! I reduced the scope of the patch. PTAL
================
Comment at: clangd/CodeComplete.cpp:1396
+ if (IndexResult && !IndexResult->IncludeHeaders.empty()) {
+ for (const auto &P : IndexResult->IncludeHeaders)
+ AddWithInclude(P);
----------------
sammccall wrote:
> I remain unconvinced that providing multiple completion candidates is the
> right thing to do here:
> - if the index hasn't seen a definition, then we're going to show one copy
> of the completion for each header that has a declaration
> - the user isn't going to have a useful way to distinguish between them.
> Note that e.g. we're going to show the #include path in the detail, but the
> documentation is going to be identical for each.
> - we lose the invariant that each completion item (pre-bundling) maps to a
> different symbol
> - C++ embedders don't have the option of displaying the multiple options
> once the completion is selected
>
> The alternative approach of sorting the includes by proximity * log(refs) or
> so, and then using the top one for scoring, seems less of a drastic change to
> the current behavior. (Visible effect: more accurate includes inserted).
These are all valid points. I agree that we should start with less drastic
change in the first version.
My concern was that there can be cases where it's impossible for clangd to
suggest the correct #include (e.g. all includes have the same proximity and
popularity). But I guess there are also alternative solutions to these than
creating multiple completion results. For example, we could simply give up
inserting include during code completion and let an include-fixer-like feature
handle it.
================
Comment at: clangd/Quality.cpp:190
+static const float kIncludeHeaderScoreIncreaseUnit = 0.0001;
+
----------------
sammccall wrote:
> This looks a bit sketchy. Particularly the += boost where everything else is
> *=.
> What's this trying to do?
(This is no longer needed in this patch.)
As we were generating multiple candidates for the same symbol (for each
include), I was trying to group them together using the small score differences
as the tie breaker.
================
Comment at: clangd/index/Merge.cpp:105
+ // merge include headers from L and R, as they can both export the symbol.
+ bool MergeIncludes = !L.Definition.FileURI.empty() &&
+ !R.Definition.FileURI.empty() &&
----------------
sammccall wrote:
> sammccall wrote:
> > This describes the logic, and the logic always produces the right result,
> > but it's not clear *why*. Maybe add something like:
> >
> > ```This is associative because it preserves the invariant:
> > - if we haven't seen a definition, Includes covers all declarations
> > - if we have seen a definition, Includes covers declarations visible from
> > any definition```
> >
> > in fact it seems hard to reason about this field in Symbol without
> > understanding this, so maybe this invariant belongs on the IncludeHeaders
> > field itself.
> Thinking more about it - what's the intent here?
> I'm not sure sorting by (seen by def, #refs) produces better ranking than
> just #refs.
>
> But there are other possible reasons for dropping includes not seen by any
> def:
> - remove spam from the completion list (only a problem if we clone the
> completion items)
> - reduce size for items that are often redeclared (I can imagine this being
> a problem, not obvious)
>
> Curious what your thinking is.
> in fact it seems hard to reason about this field in Symbol without
> understanding this, so maybe this invariant belongs on the IncludeHeaders
> field itself.
Make sense. Thanks!
> Thinking more about it - what's the intent here?
The intention is basically to filter out headers with forward declarations
where definition is not seen. I could hardly think of a case where we would
favor headers where def is not seen over those where definition is seen.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51291
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
