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