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