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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

still LG



================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:588
+    auto &NextSpelled = this->NextSpelled[File];
+
+    if (Tok.location().isFileID()) {
----------------
sammccall wrote:
> 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).
i was talking about the latter (eagerly incrementing).

With the latest diagnosing mechanism, i also agree that it should stay that way.


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:526
+    // Create empty mappings up until the end of the file.
+    for (const auto& File : Result.Files)
+      discard(File.first);
----------------
nit: formatting


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:641
 
+
   /// Initializes TokenBuffer::Files and fills spelled tokens and expanded
----------------
nit: format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77614