[PATCH] D77209: [Syntax] Add mapping from spelled to expanded tokens for TokenBuffer

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:264
+  auto *FrontMapping = mappingStartingBeforeSpelled(File, &Spelled.front());
+  unsigned SpelledFrontI = &Spelled.front() - File.SpelledTokens.data();
+  unsigned ExpandedBegin;
----------------
hlopko wrote:
> gribozavr2 wrote:
> > Or assert that SpelledFrontI is less than File.SpelledTokens.size().
> I think the assert I added is good enough?
The assertion that I suggested is stronger, because it would prevent an out-of-bounds read from even happening, and would not rely on `isBeforeInTranslationUnit` returning false on garbage data.


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:601
     assert(Result.ExpandedTokens.back().kind() == tok::eof);
+    for (auto &pair : Result.Files) {
+      auto &mappings = pair.second.Mappings;
----------------
I'd suggest moving this loop to the very bottom of this function. It is not related to the expanded token processing, which happens here (above we have assertions related to expanded tokens, and below we have the `processExpandedToken` call).

Also, `fillGapsAtEndOfFiles` below modifies mappings, it would be good if modifications were covered by the assertion as well.


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:602
+    for (auto &pair : Result.Files) {
+      auto &mappings = pair.second.Mappings;
+      assert(std::is_sorted(
----------------
We'd get an "unused variable" warning here when assertions are disabled. Please wrap the whole loop in `#ifndef NDEBUG`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77209