[PATCH] D47935: [clangd] Boost completion score according to file proximity.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

PTAL



================
Comment at: clangd/Quality.cpp:215
 
 void SymbolRelevanceSignals::merge(const CodeCompletionResult &SemaCCResult) {
   if (SemaCCResult.Availability == CXAvailability_NotAvailable ||
----------------
ioeric wrote:
> sammccall wrote:
> > proximity path needs to be set here too
> Alternatively, I wonder if we could give sema result a fixed proximity score as they are symbols that are already included? 
As discussed offline, sema symbols now have a fixed proximity score (not entirely sure about the value though).


================
Comment at: clangd/Quality.h:74
+  // If set, this is used to compute proximity from symbol's declaring file.
+  llvm::StringRef ProximityPath;
   /// Proximity between best declaration and the query. [0-1], 1 is closest.
----------------
ioeric wrote:
> ioeric wrote:
> > sammccall wrote:
> > > sammccall wrote:
> > > > It seems OK to have ProximityPath or ProximityScore, but we shouldn't have both: drop proximityscore and calculate it during evaluate()?
> > > what's the plan for associated-header? should this be a smallvector<2>?
> > Just want to make sure I understand. We would copy the symbol URI to use in `merge` right?
> I think it should be easy to change this to vector when it's actually needed?
Changed to vector anyway...


================
Comment at: clangd/Quality.h:74
+  // If set, this is used to compute proximity from symbol's declaring file.
+  llvm::StringRef ProximityPath;
   /// Proximity between best declaration and the query. [0-1], 1 is closest.
----------------
ioeric wrote:
> ioeric wrote:
> > ioeric wrote:
> > > sammccall wrote:
> > > > sammccall wrote:
> > > > > It seems OK to have ProximityPath or ProximityScore, but we shouldn't have both: drop proximityscore and calculate it during evaluate()?
> > > > what's the plan for associated-header? should this be a smallvector<2>?
> > > Just want to make sure I understand. We would copy the symbol URI to use in `merge` right?
> > I think it should be easy to change this to vector when it's actually needed?
> Changed to vector anyway...
Experimented with this a bit (removing ProximityScore). As we print the proximity score for debugging, we would still want to keep the store. Alternatively, I made the proximity paths a parameter of `merge` as we only use them for index result anyway.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47935