[PATCH] D113995: [clangd] Dex Trigrams: Improve query trigram generation

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.


================
Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:47
+  const size_t NPOS = std::numeric_limits<size_t>::max();
+  llvm::SmallVector<std::pair<size_t, size_t>> Next(LowercaseIdentifier.size());
+  size_t NextTail = NPOS, NextHead = NPOS;
----------------
sammccall wrote:
> why the change from array to pair and 0 to NPOS?
- Array of two elements is confusing: we had it since we had 3 elements, with 2 it doesn't make sense anymore. Also, makes it easier to decompose the items (`std::tie(Tail, Head)` vs `size_t Tail = Next[I][0], Head = Next[I][1]`).
- NPOS is more explicit than 0 in our intent: 0 is a valid index and it feels awkward to use a valid index as NOT_FOUND. Yes, it's the one that can't appear in the Next but it's not clear from the code and requires some explanation for understanding. Why NPOS can't be found in `Next` is obvious.


================
Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:102
+                  LowercaseIdentifier[NextHead]));
+    Position = NextHead;
+  }
----------------
sammccall wrote:
> (if the change to NPOS is just to avoid hitting Position == 0 on the first iteration, you could check it here instead)
It's not only for that but this was the trigger, having the `Position == 0` check here makes the logic slightly confusing, I think it's better to just use NPOS instead for the reasons above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113995