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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, this looks much clearer/more modular/more extensible to me!
A couple of notes on the abstractions before digging into details again.



================
Comment at: clangd/Quality.h:71
 
+class SymbolRelevanceContext {
+  public:
----------------
This is ambiguously a couple of different (and good!) things:
 - an encapsulation of the proximitypaths state and logic
 - a grouping together of the "query-dependent, symbol-invariant" inputs to the relevance calculation.

There's a place for both of these, but I'd argue for separating them (and only doing the second in this patch). Reasons:
 - the former doesn't need to be in this file if it gets complex (FuzzyMatch.h is a similar case), while the latter does
 - easier to understand/name if this hierarchy is expressed explicitly
 - I suspect we may want the context to be a separate struct, passed to SymbolRelevanceSignals::evaluate(), rather than a member of SymbolRelevanceSignals. That would add more churn than needs to be in this patch though.

If this makes sense to you then I think this class looks great but should be called something specific like `FileProximityMatcher`.


================
Comment at: clangd/Quality.h:91
 struct SymbolRelevanceSignals {
+  explicit SymbolRelevanceSignals(const SymbolRelevanceContext &Context)
+      : Context(Context) {}
----------------
One of the simplifying assumptions in the model is that all signals are optional - can we make Context a pointer `= nullptr` and drop the constructor?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47935