[PATCH] D143070: [clang-format] Enable FormatTokenSource to insert tokens.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/Format/FormatTokenSource.h:74
 public:
-  IndexedTokenSource(ArrayRef<FormatToken *> Tokens)
+  IndexedTokenSource(SmallVectorImpl<FormatToken *> &Tokens)
       : Tokens(Tokens), Position(-1) {}
----------------
klimek wrote:
> sammccall wrote:
> > As I understand it, this parameter is used:
> >  - to provide the initial set of input tokens the source will iterate over
> >  - as a common address space for input + synthesized tokens, to allow the jump mechanism to work
> >  - to make the caller responsible for ownership/lifetime of the synthesized tokens too
> > 
> > This simplifies the implementation, my only problem with this is it seems unusual and confusing.
> > A comment explaining the roles of this `Tokens` param would help a bit.
> > 
> > Alternatively you could consider slightly different data structures just for the purpose of making interfaces more obvious: e.g. pass in a `BumpPtrAllocator&`, allocate scratch tokens there, and use pointers instead of indices (jumps become a ptr->ptr map etc)
> Noticing none of this makes sense. We should really just copy the tokens, given that we modify the vector.
Oops, somehow I missed that this was an array of *pointers* and assumed that copying it was no good. This is much better.


================
Comment at: clang/lib/Format/UnwrappedLineParser.h:287
   // owned outside of and handed into the UnwrappedLineParser.
+  // FIXME: The above fixme doesn't work if we need to create tokens while
+  // parsing.
----------------
I'm not really sure how to read the combination of these two FIXMEs... does it mean "we wanted to do this differently one day, but now we never can"?

Maybe either delete both, or if you think it's still potentially actionable, something like FIXME: it would be better to have tokens created and owned outside because XYZ, but it's hard because macro expansion mutates the stream

(I don't really understand what the prev comment is about: the tokens *are* handed into the UnwrappedLineParser constructor. So I may be off base here)


================
Comment at: clang/unittests/Format/FormatTokenSourceTest.cpp:36
+    FormatToken *Tok = FormatTok;                                              \
+    EXPECT_EQ((Tok)->Tok.getKind(), tok::identifier) << *(Tok);                \
+    EXPECT_EQ((Tok)->TokenText, Name) << *(Tok);                               \
----------------
nit: parens on (Tok) are redundant given Tok is a local rather than the macro arg


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143070