[PATCH] D46524: [clangd] Extract scoring/ranking logic, and shave yaks.

[Ilya Biryukov via Phabricator via cfe-commits]

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