[PATCH] D51291: [clangd] Support multiple #include headers in one symbol.

[Eric Liu via Phabricator via cfe-commits]

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