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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/CodeComplete.cpp:909
+
+    auto QualScore = Quality.evaluate(), RelScore = Relevance.evaluate();
+    CompletionItemScores Scores;
----------------
ilya-biryukov wrote:
> 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.
Done. I was worried we might switch to double one day, seems unlikely.


================
Comment at: clangd/Quality.h:66
+  float NameMatch = 1;
+  bool Unavailable = false;
+
----------------
ilya-biryukov wrote:
> 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?
So it's `Unavailable || Inaccessible`, where neither is all that well-defined :-)
I renamed to `Forbidden` to avoid conflation with either, and added examples as a comment.


================
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:
----------------
ilya-biryukov wrote:
> 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.
Yeah, I'd rather punt on this for now to avoid creating too many tiny files and a random standard library supplement. If TopN is pulled out, llvm/Support might be a better place for it.


================
Comment at: clangd/Quality.h:119
+
+// Returns a string that sorts like (-Score, Tiebreak).
+std::string sortText(float Score, llvm::StringRef Tiebreak = "");
----------------
ilya-biryukov wrote:
> Maybe mention that it's used for LSP sortText? To give a bit more context on why we need this function.
Oops, I wrote the comment but put it in the cpp file by mistake...


================
Comment at: unittests/clangd/TestTU.cpp:62
+                      << S;
+        llvm_unreachable("QName is not unique");
+      }
----------------
ilya-biryukov wrote:
> 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.
`FAIL()` is the same as `ADD_FAILURE()` but it causes the enclosing function to immediately return - it can only be used from void functions.

> llvm_unreachable may produce surprising behaviors, i.e. miscompiles in opt mode etc, IIUC
Right. After offline discussion, we don't care about asserts-disabled test behavior if the asserts-enabled failure is sensible, so just replaced with an assert.


================
Comment at: unittests/clangd/TestTU.h:28
+  TestTU() = default;
+  TestTU(llvm::StringRef Code, llvm::StringRef HeaderCode = "")
+      : Code(Code), HeaderCode(HeaderCode) {}
----------------
ilya-biryukov wrote:
> 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);
> ```
> 
> 
> 
Done, Just added `withCode` and `withHeaderCode` for now, and anyone who wants something more complicated can set the fields directly.
(The names mirror the struct fields)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46524