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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
