[PATCH] D79918: [clangd] Don't create as much garbage while building Dex index.

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

Wow, nice work, thank you!



================
Comment at: clang-tools-extra/clangd/index/dex/Dex.cpp:51
+public:
+  // Returns the tokens which are given symbol's characteristics. Currently, the
+  // generated tokens only contain fuzzy matching trigrams and symbol's scope,
----------------
No longer "returns": "adds/generates"?


================
Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:83
+  Result.clear();
+  if (Identifier.size() < 14) {
+    identifierTrigrams(Identifier, [&](Trigram T) {
----------------
It isn't immediately obvious why processing different id lengths is different. I'm assuming it's less expensive to find duplicates in the small vector as we go than to find the unique elements in the end in case of the short identifier, but I'd be happy to have a comment here.

Also, `14` is a magic number:

* Move it to `Trigram::SMALL_IDENTIFIER_SIZE` or some other constant?
* How did you find this number? Did you try running benchmark with different values and figured this one is the best for the LLVM index? If not, it would make sense to do that.


================
Comment at: clang-tools-extra/clangd/index/dex/Trigram.h:49
+  std::string str() const { return std::string(Data.data(), Data[3]); }
+  uint32_t uint() const { return llvm::bit_cast<uint32_t>(Data); }
+  friend struct ::llvm::DenseMapInfo<Trigram>;
----------------
nit: maybe call it `UID` or something similar, "unsigned int" is somewhat surprising in this context


================
Comment at: clang-tools-extra/clangd/index/dex/Trigram.h:55
+
 /// Returns list of unique fuzzy-search trigrams from unqualified symbol.
 /// The trigrams give the 3-character query substrings this symbol can match.
----------------
Also doesn't "return" anymore


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79918/new/

https://reviews.llvm.org/D79918