[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:
> > 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.


================
Comment at: lib/Format/WhitespaceManager.cpp:401
+  return Current->is(tok::identifier) && Current->Next &&
+         Current->Next->SpacesRequiredBefore == SpacesRequiredBefore;
+}
----------------
Note that this is comparing an unsigned with a bool, which might not work as you expect. And especially, there are tokens for which we set SpacesRequiredBefore to 2. Also I wonder whether we should check the SpacesRequiredBefore of the r_paren in a function like macro, i.e. in

  #define a(x) (x)

We should check that a space is required between "a(x)" and "(x)". Please take a look at my longer code snippet below where I am proposing an alternative solution.


https://reviews.llvm.org/D28462