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

[Erik Nyquist via Phabricator via cfe-commits]

enyquist marked 2 inline comments as done.
enyquist 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))
----------------
djasper wrote:
> 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.
Great, thanks for fixing. Your change works.


================
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
----------------
djasper wrote:
> 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;
I'm not sure exactly what you mean. Do you mean, why do I need this special case to ignore scope changes and commas? This was the only way I could get it to work, AlignTokens was bailing out as soon as a paren or a comma inside parens was seen.


Repository:
  rL LLVM

https://reviews.llvm.org/D28462