sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land.
Nice, simple and will admit refinements later. Just test nits and a trivial organizational thing. ================ Comment at: clangd/Quality.cpp:22 +namespace { +bool hasDeclInMainFile(const Decl &D) { ---------------- nit: per coding style use static for functions (I'm not sure it's a great rule, but since the ns only has this function...) ================ Comment at: clangd/Quality.h:52 unsigned References = 0; + float ProximityScore = 0.0; /// Proximity score, in a [0,1] interval. ---------------- this belongs in `SymbolRelevanceSignals` rather than this struct. In principle, SymbolQualitySignals should all be stuff we can store in a global index (which is the point of the split). I should probably add a comment to that effect :-) You could be a little more specific on the semantics, e.g. "Proximity between the best declaration and the query location. [0-1] score where 1 is closest." ================ Comment at: unittests/clangd/QualityTests.cpp:96 TEST(QualityTests, SymbolRelevanceSignalsSanity) { SymbolRelevanceSignals Default; ---------------- please add a test for proximity here ================ Comment at: unittests/clangd/QualityTests.cpp:121 +TEST(QualityTests, BoostCurrentFileDecls) { + TestTU Test; ---------------- consider merging into SymbolRelevanceSignalsExtraction test, which tests the same entrypoint. If not, move up next to that one. ================ Comment at: unittests/clangd/QualityTests.cpp:129 + Test.Code = R"cpp( + #include "foo.h" + int ::test_func_in_header_and_cpp() { ---------------- this is not needed, the `#include` is implicit in TestTU (and so you don't need to specify HeaderFilename either) ================ Comment at: unittests/clangd/QualityTests.cpp:155 + + /// Check the boost in proximity translates into a better score. + EXPECT_LE(FuncInHeader.evaluate(), FuncInCpp.evaluate()); ---------------- this tests end-to-end, but the other tests verify input -> signals and signal -> score separately. I'd prefer to keep (only) doing that, for consistency and because it's important we know/assert precisely what each half does so we can actually debug. ================ Comment at: unittests/clangd/TestTU.cpp:80 continue; - if (Result) { + if (Result && ND->getCanonicalDecl() != Result) { ADD_FAILURE() << "Multiple Decls named " << QName; ---------------- well, I definitely wanted to flag this as an error (for the tests where this function was introduced). Actually I think this is wrong for your test anyway - don't you want to test that no matter which decl the code completion result refers to, you get the same boost? I'd suggest adding a `findDecls()` function that returns a vector<NamedDecl*>, and implementing `findDecl()` in terms of it. In the `test_func_in_header_and_cpp` test, you could loop over `findDecls()` and run the assertions each time. (I guess findDecls() should assert that the returned vector is non-empty? Slightly weird but might catch accidentally vacuous tests) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46943 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits