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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

The new uploaded diff has lots of unrelated changes to clang-tidy, clang-move, etc...
Looking at commits, it seems `arc diff` was called with the wrong base commit... 
Could you please reupload the change?



================
Comment at: clangd/Quality.h:45
+
+// Attributes of a symbol that affect how much we like it.
+struct SymbolQualitySignals {
----------------
ilya-biryukov wrote:
> Maybe use doxygen-style comments to be consistent with the rest of LLVM?
Some changes are missing?
File still uses 2-slash instead of 3-slash comments.


================
Comment at: clangd/Quality.h:66
+  float NameMatch = 1;
+  bool Unavailable = false;
+
----------------
sammccall wrote:
> 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.
Thanks! `Forbidden` with a comment LG.


================
Comment at: unittests/clangd/TestTU.h:28
+  TestTU() = default;
+  TestTU(llvm::StringRef Code, llvm::StringRef HeaderCode = "")
+      : Code(Code), HeaderCode(HeaderCode) {}
----------------
sammccall wrote:
> 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)
LG, thanks.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46524