sammccall added a comment.
Thanks, just details now!
================
Comment at: clangd/Quality.cpp:185
+ }
+ if (F == Fs.begin() && T == Ts.begin()) // No common directory.
+ return 0;
----------------
why is this a special case?
- /x/a/b vs /x/a/c is 1 up + 1 down --> 0.59
- /a/b vs /a/c is 1 up + 1 down --> 0.59
- /b vs /c is unrelated --> 0
I don't doubt the assertion that these are unrelated paths, but I'm not sure
fixing just this case is an improvement overall. (In a perfect world, we'd
define the algorithm so that this case yields 0 without a discontinuity)
================
Comment at: clangd/Quality.cpp:216
+ const FileProximityMatcher &M) {
+ OS << formatv("=== File proximity matcher ===\n");
+ OS << formatv(
----------------
For composability, you could consider styling more tersely e.g. as
ProximityPaths{/path/to/file}, and in the RelevanceSignals operator<< including
it like other fields, yielding:
```
== Symbol relevance: 0.8 ==
Name match: 0.7
File proximity matcher: ProximityPaths{/path/to/file}
...
```
================
Comment at: clangd/Quality.h:78
+
+ /// Calculates the best proximity score from proximity paths to the
symbol's
+ /// URI. When a path cannot be encoded into the same scheme as \p
SymbolURI,
----------------
Should mention the semantics of the score, maybe via the other extreme: when
the SymbolURI exactly matches a proximity path, score is 1.
================
Comment at: clangd/Quality.h:98
+ /// closest.
+ float IndexProximityScore = 0;
+ llvm::StringRef IndexSymbolURI;
----------------
This is redundant with (IndexSymbolURI, FileProximityMatch) I think, and will
only be correctly set if FileProximityMatch is set before calling merge(Symbol).
Can we defer calculation until evaluate()?
(If you want to dump this intermediate score, you can recompute it in
operator<<, I'm not sure it's necessary).
================
Comment at: unittests/clangd/TestFS.cpp:66
+// Assume all files in the schema have a "test-root/" root directory, and the
+// schema path is the relative path to the root directory.
----------------
These helpers would be more coherent if this used the same test root as above -
any reason we can't do that?
Then this comment could just be "unittest: is a scheme that refers to files
relative to testRoot()"
================
Comment at: unittests/clangd/TestFS.cpp:107
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the plugin.
----------------
This is really surprising to me - is this the common pattern for registries?
(i.e. we don't have something more declarative like bazel's
`cc_library.alwayslink`)?
If so, can we move the declaration to TestFS.h and give a usage example, so the
consuming libraries don't have to repeat the decl?
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
