[PATCH] D49591: [clangd] Introduce search Tokens and trigram generation algorithms

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thanks for the polish!
Last few nits, but feel free to land once you're happy.



================
Comment at: clang-tools-extra/clangd/index/dex/Token.cpp:21
+
+Token::Token(Kind TokenKind, llvm::StringRef Data)
+    : Data(Data), TokenKind(TokenKind) {}
----------------
nit: I'd suggest inlining these trivial functions and deleting the cpp file - one less file is always nice!


================
Comment at: clang-tools-extra/clangd/index/dex/Token.h:51
+    ///
+    /// Data stroes full scope name is stored, e.g. "foo::bar::baz::" or ""
+    /// (global scope).
----------------
This sentence got a bit mangled.


================
Comment at: clang-tools-extra/clangd/index/dex/Token.h:90
+  static inline clang::clangd::dex::Token getEmptyKey() {
+    static clang::clangd::dex::Token EmptyKey(
+        clang::clangd::dex::Token::Kind::Sentinel, "EmptyKey");
----------------
nit: just `return {clang::clangd::dex::Token::Kind::Sentinel, "EmptyKey"};`
you're making a copy here anyway.


================
Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:56
+  for (int I = LowercaseIdentifier.size() - 1; I >= 0; --I) {
+    Next[I] = {{NextTail, NextHead, NextNextHead}};
+    NextTail = Roles[I] == Tail ? I : 0;
----------------
(just for my curiosity - why are the double braces needed rather than single?)


================
Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:101
+
+  std::string LowercaseQuery = Query;
+  for (auto &Character : LowercaseQuery)
----------------
nit: `LowercaseQuery = Query.lower();`
(and above)


================
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:22
+
+std::vector<Token> getTrigrams(std::initializer_list<std::string> Trigrams) {
+  std::vector<Token> Result;
----------------
you never call this function from tests, fold it together into `Trigrams`?


================
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:31
+testing::Matcher<std::vector<Token>>
+Trigrams(std::initializer_list<std::string> Elements) {
+  return testing::UnorderedElementsAreArray(getTrigrams(Elements));
----------------
nit: function needs to start with a lowercase letter

if you like: `trigramsAre` would fit with typical matcher factory style


https://reviews.llvm.org/D49591