ilya-biryukov added inline comments.
================
Comment at: clangd/Quality.h:52
unsigned References = 0;
+ float ProximityScore = 0.0; /// Proximity score, in a [0,1] interval.
----------------
sammccall wrote:
> 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."
Hah, I totally missed the `SymbolRelevanceSignals`. Thanks, moved it there and
added a comment per suggestions.
================
Comment at: unittests/clangd/QualityTests.cpp:121
+TEST(QualityTests, BoostCurrentFileDecls) {
+ TestTU Test;
----------------
sammccall wrote:
> consider merging into SymbolRelevanceSignalsExtraction test, which tests the
> same entrypoint.
>
> If not, move up next to that one.
Merged them together. It is now a bit more verbose. In case you have more
suggestions on how to properly test this, I'm happy to address them in a
follow-up change.
================
Comment at: unittests/clangd/QualityTests.cpp:129
+ Test.Code = R"cpp(
+ #include "foo.h"
+ int ::test_func_in_header_and_cpp() {
----------------
sammccall wrote:
> this is not needed, the `#include` is implicit in TestTU
>
> (and so you don't need to specify HeaderFilename either)
Done.
I did not expect the `TestTU` to do that, actually. Is this implicit `#include`
something we want?
Maybe we should just have a default convention for naming the headers instead
and the default `Code` value that only includes the header?
E.g. say that a file `test_header.h` is provided by TestTU and let the tests
specify `#include "test_header.h"` if needed. WDYT?
================
Comment at: unittests/clangd/TestTU.cpp:80
continue;
- if (Result) {
+ if (Result && ND->getCanonicalDecl() != Result) {
ADD_FAILURE() << "Multiple Decls named " << QName;
----------------
sammccall wrote:
> 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)
This function got reimplemented in one of the recent changes and works for
header decls too now.
I actually think it's fine if it returns one of the overladed Decls in case
there are multiple overloads (i.e. multiple Decls in the AST) for the **same**
function. It does assert for multiple overloads of the same thing currently,
which is probably what we want.
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
