[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:239
+
+  auto It = llvm::partition_point(F.Mappings, [SpelledI](const Mapping &M) {
+    return M.BeginSpelled <= SpelledI;
----------------
It would be great to add an is_sorted assertion to the builder to check ordering of mappings based on both spelled and expanded token indices.


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:259
+  const MarkedFile &File = It->second;
+  // assert that Spelled is a subarray of File.SpelledTokens.
+  assert(File.SpelledTokens.data() <= Spelled.data());
----------------
s/subarray/subrange/

Also no need to repeat "assert".

"`Spelled` must be a subrange of `File.SpelledTokens`."


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:261
+  assert(File.SpelledTokens.data() <= Spelled.data());
+  assert(Spelled.size() <= File.SpelledTokens.size());
+
----------------
I think we can improve the precision of this assertion. Comparing the sizes does not ensure that we really have a subrange. I think the second assert should be comparing end pointers instead of sizes. Something like:

`assert(&Spelled.back() <= &File.SpelledTokens.back());`


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:264
+  auto *FrontMapping = mappingStartingBeforeSpelled(File, &Spelled.front());
+  unsigned SpelledFrontI = &Spelled.front() - File.SpelledTokens.data();
+  unsigned ExpandedBegin;
----------------
Or assert that SpelledFrontI is less than File.SpelledTokens.size().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77209