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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
