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