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

[Sam McCall via Phabricator via cfe-commits]

sammccall 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;
----------------
you've changed unsigned -> size_t here, can you revert that please?


================
Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:91
+    std::tie(NextTail, NextHead) = Next[Position];
+    if (NextTail != NPOS)
+      Out(Trigram(LowercaseIdentifier[Position],
----------------
You're not treating head vs tail differently, this should just be iterating over Next as above.


================
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;
----------------
kbobyrev wrote:
> 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.
I disagree on the style points here and they don't seem particularly related to the patch. Do they need to be included here?

> Array of two elements is confusing

Well, I disagree here: the difference between array<T, 2> and pair<T, T> is homogeneous vs heterogeneous. The usage here is almost entirely homogeneous.
3 vs 2 doesn't make a difference, we could have used `tuple<T, T, T>`, same would apply.

> Also, makes it easier to decompose the items

As far as ergonomics go, this comes at the cost of not being able to iterate over them, which we do much more often. (And AFAICT you never need NextTail decomposed.)

`NPOS` is not a common convention so I don't think it's more obvious. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113995