[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

[Ilya Biryukov via Phabricator via cfe-commits]

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