ilya-biryukov added a comment.
The change makes both testing and scoring code better.
Even though those are largely independent changes, perfectly happy to review
them together.
I mostly have NITs here, overall the changes LG.
================
Comment at: clangd/CodeComplete.cpp:36
+#define DEBUG_TYPE "codecomplete"
+
----------------
Is this used by `DEBUG()` macro?
It would be nice to have a short comment saying why we need this define.
================
Comment at: clangd/CodeComplete.cpp:909
+
+ auto QualScore = Quality.evaluate(), RelScore = Relevance.evaluate();
+ CompletionItemScores Scores;
----------------
NIT: Maybe use `float` here instead of auto? Would remove the need to look at
`evaluate` for anyone reading the code for the first time.
================
Comment at: clangd/Quality.cpp:83
+// That is: a < b <==> encodeFloat(a) < encodeFloat(b).
+uint32_t encodeFloat(float F) {
+ static_assert(std::numeric_limits<float>::is_iec559, "");
----------------
This function does not seem to be exposed outside this C++ file. Maybe add
`static`?
================
Comment at: clangd/Quality.h:45
+
+// Attributes of a symbol that affect how much we like it.
+struct SymbolQualitySignals {
----------------
Maybe use doxygen-style comments to be consistent with the rest of LLVM?
================
Comment at: clangd/Quality.h:66
+ float NameMatch = 1;
+ bool Unavailable = false;
+
----------------
Maybe rename to `Inaccessible`? It seems to be closer to what this bool means
in C++, if I'm reading the code correctly.
Or add a comment explaining what "unavailable" means?
================
Comment at: clangd/Quality.h:80
+// TopN<T> is a lossy container that preserves only the "best" N elements.
+template <typename T, typename Compare = std::greater<T>> class TopN {
+public:
----------------
As you mentioned in the change description, moving `TopN` and `sortText` to a
separate file might be a good idea since they don't depend on various
clangd-specific bits.
But I'm perfectly happy to leave it as is and do this later if needed.
================
Comment at: clangd/Quality.h:119
+
+// Returns a string that sorts like (-Score, Tiebreak).
+std::string sortText(float Score, llvm::StringRef Tiebreak = "");
----------------
Maybe mention that it's used for LSP sortText? To give a bit more context on
why we need this function.
================
Comment at: unittests/clangd/TestTU.cpp:56
+ const Symbol *Result = nullptr;
+ for (const Symbol &S : Slab)
+ if (QName == (S.Scope + S.Name).str()) {
----------------
Maybe add braces for the loop body here? It seems to be long enough that it
could actually, arguably, improve readability.
We could some nesting by inverting the condition too:
`if (QName != ...) continue;`
================
Comment at: unittests/clangd/TestTU.cpp:62
+ << S;
+ llvm_unreachable("QName is not unique");
+ }
----------------
Maybe use `FAIL()` instead of `ADD_FAILURE()` followed by `llvm_unreachable()`?
`llvm_unreachable` may produce surprising behaviors, i.e. miscompiles in opt
mode etc, IIUC.
Both failing or returning a first matching symbol while also signalling an
error seem like good alternatives here.
================
Comment at: unittests/clangd/TestTU.h:28
+ TestTU() = default;
+ TestTU(llvm::StringRef Code, llvm::StringRef HeaderCode = "")
+ : Code(Code), HeaderCode(HeaderCode) {}
----------------
I really like this helper, now that we can reuse the code between different
tests!
It took me some time to get the semantics of this constructor, though.
I suggest to have a few constructor functions with more descriptive name (my
names are not great, but should give the idea):
```
static TestTU FromSourceFile(StringRef Code);
static TestTU FromHeaderFile(StringRef Code);
static TestTU WithImplicitInclude(StringRef Source, StringRef IncludedHeader);
```
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D46524
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
