[PATCH] D113995: [clangd] Dex Trigrams: Improve query trigram generation
Kirill Bobyrev via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Nov 30 02:02:03 PST 2021
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;
> 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], Head = Next[I]`).
- 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
+ Position = NextHead;
> (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.
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the cfe-commits