[PATCH] D77614: [Syntax] Simplify TokenCollector::Builder, use captured expansion bounds. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 4 inline comments as done.
sammccall added inline comments.


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:588
+    auto &NextSpelled = this->NextSpelled[File];
+
+    if (Tok.location().isFileID()) {
----------------
kadircet wrote:
> nit: maybe create mapping here and increment NextSpelled and NextExpanded explicitly here.
Not quite sure what you mean here, my guesses...

Hoisting mapping creation to here: If we have file tokens, we're not creating a mapping. Only nontrivial mappings are stored (where the spelled and expanded locations differ). This may be a design mistake but it's not one I'm fixing in this patch :-)

Incrementing NextSpelled and NextExpanded eagerly: if our invariants hold (expanded and spelled tokens really do correspond) then we will indeed increment each of these at least once, so we could structure the code that way. However those invariants are ridiculously subtle and fragile (basically depends on the correctness of the TokenWatcher impl in Preprocessor) so in practice it's good not to advance if our assumptions aren't met so we can actually debug the result. The latest version of the patch makes use of this to detect and crash with a useful message (diagnoseAdvanceFailure).


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:594
+             SpelledTokens[NextSpelled].location() ==
+                 Result.ExpandedTokens[NextExpanded].location()) {
+        ++NextSpelled;
----------------
kadircet wrote:
> maybe also explicitly spell the assumption:
> `assert(ExpandedTok.location().isFileID())` ?
This is very weak compared to the explicitly checked equality in the while loop, which I think spells out the assumption better (these are consecutive spelled tokens).

That assertion would effectively be checking that SpelledTokens doesn't contain any tokens with macro locations, which is trivially true by construction and not really sensible to check in all the places we rely on it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77614