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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Nice!

We could reduce the scope of this patch somewhat by ignoring file proximity and just switching to return the most popular header. This would be a solid improvement over current behavior, and provide the infrastructure for the file-proximity approach.



================
Comment at: clangd/CodeComplete.cpp:1396
+      if (IndexResult && !IndexResult->IncludeHeaders.empty()) {
+        for (const auto &P : IndexResult->IncludeHeaders)
+          AddWithInclude(P);
----------------
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).


================
Comment at: clangd/Quality.cpp:190
 
+static const float kIncludeHeaderScoreIncreaseUnit = 0.0001;
+
----------------
This looks a bit sketchy. Particularly the += boost where everything else is *=.
What's this trying to do?


================
Comment at: clangd/index/Index.h:220
+    llvm::StringRef IncludeHeader = "";
+    /// The number of translation units that reference this symbol via this
+    /// header. This number is only meaningful if aggregated in an index.
----------------
via this header -> and include this header?

(otherwise, what does "via" mean?)


================
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() &&
----------------
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.


================
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:
> 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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51291