[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.


================
Comment at: lib/Format/WhitespaceManager.cpp:413
+
+  while (Param && !Param->is(tok::l_paren)) {
+    if (!Param->is(tok::identifier) && !Param->is(tok::comma))
----------------
enyquist wrote:
> djasper wrote:
> > enyquist wrote:
> > > djasper wrote:
> > > > I think you should be able to use Current.MatchingParen here.
> > > Hmm, I couldn't make this work... I just replaced this line with
> > > 
> > >     while (Param && Param != Current->MatchingParen)
> > > 
> > > But it must not be doing what I think it's doing, since it made some tests fail.
> > > Mind you, my C-brain might be taking over here, please let me know if I'm using MatchingParen incorrectly
> > You shouldn't need a while loop. Just:
> > 
> >   if (!Current->MatchingParen || !Current->MatchingParen->Previous)
> >     return false;
> >   Param = Param->MatchingParen->Previous;
> > 
> > And with that, I think you can simplify all these methods a bit:
> > 
> >   static bool endsPPIdentifier(const FormatToken *Current) {
> >     if (!Current->Next || Current->Next->SpacesRequiredBefore == 0)
> >       return false;
> > 
> >     // If Current is a "(", skip past the parameter list.
> >     if (Current->is(tok::r_paren)) {
> >       if (!Current->MatchingParen || !Current->MatchingParen->Previous)
> >         return false;
> >       Current = Current->MatchingParen->Previous;
> >     }
> > 
> >     const FormatToken *Keyword = Current->Previous;
> >     if (!Keyword || !Keyword->is(tok::pp_define))
> >       return false;
> > 
> >     return Current->is(tok::identifier);
> >   }
> > 
> > Does that work? If so, I'd even suggest inlining this code directly into the lambda instead of pulling out a function.
> It seems that MatchingParen does not get set for parens surrounding a macro-function parameter list. So for now, a loop is needed. I was able to clean it up, though, and I've inlined the whole thing in the lambda.
Fixed that in r300661. We really should not add more ways to parse parens, especially not outside of unwrapped lines (e.g. in the whitespace manager). That can lead to very bad situations for error recovering when the code is incomplete.


================
Comment at: lib/Format/WhitespaceManager.cpp:400
+
+                const FormatToken *Current = (&C - 1)->Tok;
+                unsigned SpacesRequiredBefore = 1;
----------------
No. Don't use pointer magic to solve this. Come up with a way to properly iterate through the changes so this isn't needed.


================
Comment at: lib/Format/WhitespaceManager.cpp:431
+
+              // Special case for AlignTokens: for all other alignment cases,
+              // the current sequence is ended when a comma or a scope change
----------------
I am not yet sure I understand this. How is this different from:

  $ clang-format test.cc -style="{AlignConsecutiveDeclarations: true}"
  map<int, int> m;
  map<int>      m;
  int           a;


Repository:
  rL LLVM

https://reviews.llvm.org/D28462