ilya-biryukov added inline comments.
================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:130
+ OS << llvm::formatv(
+ " ['{0}'_{1}, '{2}'_{3}) => ['{4}'_{5}, '{6}'_{7})\n",
+ PrintToken(File.SpelledTokens[M.BeginSpelled]), M.BeginSpelled,
----------------
sammccall wrote:
> sammccall wrote:
> > As predicted :-) I think these `_<index>` suffixes are a maintenance hazard.
> >
> > In practice, the assertions are likely to add them by copy-pasting the test
> > output.
> >
> > They guard against a class of error that doesn't seem very likely, and in
> > practice they don't even really prevent it (because of the copy/paste
> > issue).
> >
> > I'd suggest just dropping them, and living with the test assertions being
> > slightly ambiguous.
> >
> > Failing that, some slightly trickier formatting could give more context:
> >
> > `A B C D E F` --> `A B ... E F`
> >
> > With special cases for smaller numbers of tokens. I don't like the
> > irregularity of that approach though.
> (this one is still open)
Will make sure to do something about it before submitting
================
Comment at: clang/unittests/Tooling/Syntax/TokensTest.cpp:622
+
+// FIXME: add tests for mapping spelled tokens into offsets.
+
----------------
sammccall wrote:
> sammccall wrote:
> > please fix :-)
> (still missing?)
Will make sure to land this before submitting.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59887/new/
https://reviews.llvm.org/D59887
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
