clang-format: Add new style option AlignConsecutiveMacros

[VelocityRa]

Summary

This option behaves similarly to AlignConsecutiveDeclarations and AlignConsecutiveAssignments, aligning the assignment of C/C++ preprocessor macros on consecutive lines.

I've worked in many projects (embedded, mostly) where header files full of large, well-aligned "#define" blocks are a common pattern. We normally avoid using clang-format on these files, since it ruins any existing alignment in said blocks. This style option will align "simple" PP macros (no parameters) and PP macros with parameter lists on consecutive lines.

Related Bugzilla entry (thanks mcuddie): https://llvm.org/bugs/show_bug.cgi?id=20637

[djasper]

unittests/Format/FormatTest.cpp

7521 ↗

(On Diff #93092)

I think it might be useful to create tests for multi-line macros.

7522 ↗

(On Diff #93092)

Either call this "Style" instead of "Alignment" or create two styles, Alignment and NoAlignment.

7733 ↗

(On Diff #93092)

Why this change? This seems to be tested above.

8010 ↗

(On Diff #93092)

Same here?

[enyquist]

lib/Format/WhitespaceManager.cpp

287 ↗

(On Diff #93092)

Currently, they are never set independently, no. I will combine them and add an explanatory comment.

413 ↗

(On Diff #93092)

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

unittests/Format/FormatTest.cpp

7733 ↗

(On Diff #93092)

I wanted to ensure this option didn't break other alignment options while I was developing-- this is a remnant. I will remove it.

8010 ↗

(On Diff #93092)

Yes.

[enyquist]

Addressed all comments, except for the one about FormatToken.MatchingParen (see reply comment)

[djasper]

lib/Format/WhitespaceManager.cpp

401 ↗

(On Diff #93204)

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.

413 ↗

(On Diff #93092)

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.

[enyquist]

lib/Format/WhitespaceManager.cpp

413 ↗

(On Diff #93092)

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.

[enyquist]

Apologies-- forgot to update a comment

[enyquist]

Rebased on latest

[djasper]

lib/Format/WhitespaceManager.cpp

400 ↗

(On Diff #95680)

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.

431 ↗

(On Diff #95680)

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;

413 ↗

(On Diff #93092)

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.

[enyquist]

lib/Format/WhitespaceManager.cpp

431 ↗

(On Diff #95680)

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.

413 ↗

(On Diff #93092)

Great, thanks for fixing. Your change works.

[enyquist]

Addressed comments

[djasper]

lib/Format/WhitespaceManager.cpp

431 ↗

(On Diff #95680)

Yes, that's what I mean. The example I am writing above works correctly and structurally, the two should be the same, right? Maybe this is easier now that you can skip over the parameter list because of the correct MatchingParen?

[enyquist]

lib/Format/WhitespaceManager.cpp

431 ↗

(On Diff #95680)

Hmm... when I just set IgnoreScopeAndCommas to False, most of the tests fail (all the blocks with parens in them) seem to fail. Sample:

/home/enyquist/clang-llvm/llvm/tools/clang/unittests/Format/FormatTest.cpp:74: Failure
      Expected: Code.str()
      Which is: "#define a    3\n#define bbbb 4\n#define ccc  (5)"
To be equal to: format(test::messUp(Code), Style)
      Which is: "#define a 3\n#define bbbb 4\n#define ccc (5)"
With diff:
@@ -1,3 +1,3 @@
-#define a    3
+#define a 3
 #define bbbb 4
-#define ccc  (5)
+#define ccc (5)

Not sure exactly what's going on here, I'll look into it. Perhaps I don't need to worry about commas.

[enyquist]

Gaaah. I'm so sorry. I wrote that last comment months ago and never submitted. No wonder you guys weren't responding.

lib/Format/WhitespaceManager.cpp

470 ↗

(On Diff #96333)

@djasper, this is my reasoning for the special case (whether or not it's a good reason, is open to discussion, but this is the reason anyway):
AlignConsecutiveDeclarations only needs to look at one token, since the information required to determine a suitable token for this alignment is provided with the FormatToken. So, it doesn't matter if the current set of changes only contains one token (because, say, the previous portion was in a diff. scope and handled by a recursive invocation of AlignTokens).

However, in order to determine an appropriate token in a PP macro, since these tokens are not annotated with the required information, the lambda function must attempt to crawl through the whole statement back to the '#define' keyword (meaning, all those tokens need to be processed in a single invocation of AlignTokens, without recurring).

The best alternative I could see for this was to just annotate the FormatTokens with the needed info, which was in my initial implementation of this patch, however this adds unconditional overhead as you know.

[sylvestre.ledru]

Any news on this?
cc @djasper

[jzr]

I happen to need this functionality as well. What's the progress? Is there anything I can do to help it along?

[jzr]

On a related note, would it be difficult to add support for custom spacing between the macro name and definition?
In the HelenOS project, we have definitions aligned two spaces after the longest name. It helps readability considerably.

[dtzWill]

Ping! Is this stalling on reviewer attention? If so, please submit to LLVM Weekly's review corner (assuming it works with current LLVM), see http://llvmweekly.org/reviewcorner for details.

[kromanova]

We have a request for this feature in clang-format in Sony.

[jorisa]

This is also a highly anticipated feature for us!

[enyquist]

Rebased on current master. No functional changes, just a minor conflict where a variable got re-named

[enyquist]

@dtzWill thanks for the suggestion, I have submitted this change to the weekly review corner.

[• elvinio]

Any updates on this request? Eagerly waiting for this feature.

[markoshorro]

This feature should have been merged time ago, any news?

[enyquist]

As far as I know, there are no updates required from me for this pull request-- I rebased on the main trunk recently, and will do it again tonight to be sure. So it should be compiling/working just fine.
I believe it is just awaiting final approval from somebody.

[TimF]

In D28462#1064789, @enyquist wrote:

As far as I know, there are no updates required from me for this pull request-- I rebased on the main trunk recently, and will do it again tonight to be sure. So it should be compiling/working just fine.
I believe it is just awaiting final approval from somebody.

I have done a checkout of both trunks yesterday, applied your patch and compiled everything. It is working flawlessly and now the defines are correctly aligned. I still hope they'll merge it because I prefer using an official release, but until then it will do the job.
Thanks a lot for your work on this feature, it was the last point preventing us from using clang-format. (embedded development too, so we have lots of defines). And thanks to all the LLVM contributors by the way :-)

[• bartpeeters]

We too would like this feature to be in the official release

[klimek]

lib/Format/WhitespaceManager.cpp

438 ↗

(On Diff #135744)

I'm not sure whether we should use AlignTokens here, given that we pass in a parameter to basically skip all its interesting logic.

What I'd personally do is try to implement the alignment of macros on their own merit from scratch, which should be significantly simpler, and then look for whether we can pull out common functions between AlignTokens and that implementation to further reduce duplication.

[tiena2cva]

I am waiting for this feature. Any update?

[• RKeelan]

My team is waiting for this feature, too.

[enyquist]

@klimek fair point. To be honest, I've pretty much lost interest / momentum on this feature, I very much doubt I will ever go back and re-implement from scratch as you suggest.
Not meaning to sound rude, I just don't want to waste anyone's time who is waiting for this (seems there are a few).

[klimek]

In D28462#1147019, @enyquist wrote:

@klimek fair point. To be honest, I've pretty much lost interest / momentum on this feature, I very much doubt I will ever go back and re-implement from scratch as you suggest.
Not meaning to sound rude, I just don't want to waste anyone's time who is waiting for this (seems there are a few).

I do agree this is frustrating - I'd personally really like this feature, but having an implementation that fundamentally matches the architecture of the code is important for long-term maintenance and sustainability of the project. This is unfortunately a hard / lots-of-work feature to get right. I do appreciate the time you took to look into this.

[enyquist]

@klimek having gotten that out of the way, I do occasionally drink too much and have sudden urges to re-implement things from scratch. Close it if you need to, since I can't commit to anything, but.... it it happens to be still open on one of those nights, who knows, maybe I'll end up doing it :)

[klimek]

In D28462#1148732, @enyquist wrote:

@klimek having gotten that out of the way, I do occasionally drink too much and have sudden urges to re-implement things from scratch. Close it if you need to, since I can't commit to anything, but.... it it happens to be still open on one of those nights, who knows, maybe I'll end up doing it :)

Lol, let's hope you'll hit the Ballmer Peak at one of these nights ;)

[lassi.niemisto]

My project team would also highly appreciate if this got implemented. Currently working around the issue by post-formatting the clang-formatted files with uncrustify and filtering it so that it only touches lines starting with #.

[mewmew]

Any update on this? There are quite a few people who got excited about this change and would like to start using it with clang-format.

[smilewithani]

In D28462#1257259, @mewmew wrote:

Any update on this? There are quite a few people who got excited about this change and would like to start using it with clang-format.

Same here!

[enyquist]

@smilewithani @lassi.niemisto @mewmew feel free to take a stab at it. Read the comments on this page right here to see what is required specifically.

[AlexAltea]

My open-source project would heavily benefit from clang-format, but we require this proposed AlignConsecutiveMacros feature.
I've placed a modest bounty of 50 USD for it. Since BountySource isn't compatible with Phabricator, I linked the bounty to an "external" issue from my repository.

More info at: https://www.bountysource.com/issues/68844865-add-style-option-alignconsecutivemacros-to-clang-format

[VelocityRa]

I extracted the logic of AlignTokens and AlignTokenSequence that aligning macros uses to the best of my ability.

This new logic now resides in AlignTokensAll and AlignTokenSequenceAll (so that it may be re-used).

As far as re-using pieces of the implementation of those two with the former methods, I couldn't spot any patterns I could extract, that wouldn't make the code needlessly complicated (in my opinion).

Tests pass, awaiting review.

[MyDeveloperDay]

I checked this out and built this and it works really well. The thing I really like is that it helps to format Windows resource include files e.g. 'Resource.h' better than before.

Which changes the current clang-formatted version

#define IDD_AAAAAAAAAAAAAAAAAAAA 101
#define IDS_BBBBBBBBBBBBBBB 101
#define IDR_CCCCCCCCCCCCCCC 102
#define IDB_DDDDDDDDDDDDDDDDDDDDDDDDDDDD 103
#define IDS_EEEEEEEEEEEEEEEEEE 104
#define IDR_FFFFFFFFFFFFFFFFFFFFFFFFFFFF 106
#define IDD_GGGGGGGGGGGGGGGGGGGGGGGGGGGGGG 107
#define IDD_HHHHHHHHHHHHHHHHHHHHHHHHHHHHH 108
#define IDS_IIIIIIIIIIIIIIIIIIIII 111
#define IDS_JJJJJJJJJJJJJJJJJJJJJJJJ 112
#define IDS_KKKKKKKKKKKKKKKKKKK 114
#define IDD_LLLLLLLLLLLLLLLL 115

to

#define IDD_AAAAAAAAAAAAAAAAAAAA           101
#define IDS_BBBBBBBBBBBBBBB                101
#define IDR_CCCCCCCCCCCCCCC                102
#define IDB_DDDDDDDDDDDDDDDDDDDDDDDDDDDD   103
#define IDS_EEEEEEEEEEEEEEEEEE             104
#define IDR_FFFFFFFFFFFFFFFFFFFFFFFFFFFF   106
#define IDD_GGGGGGGGGGGGGGGGGGGGGGGGGGGGGG 107
#define IDD_HHHHHHHHHHHHHHHHHHHHHHHHHHHHH  108
#define IDS_IIIIIIIIIIIIIIIIIIIII          111
#define IDS_JJJJJJJJJJJJJJJJJJJJJJJJ       112
#define IDS_KKKKKKKKKKKKKKKKKKK            114
#define IDD_LLLLLLLLLLLLLLLL               115

But alas it will still fight with Visual Studio which wants to align them with its 40 characters or 1+ more than the #define length strategy (e.g.)

#define IDD_AAAAAAAAAAAAAAAAAAAA        101
#define IDS_BBBBBBBBBBBBBBB             101
#define IDR_CCCCCCCCCCCCCCC             102
#define IDB_DDDDDDDDDDDDDDDDDDDDDDDDDDDD 103
#define IDS_EEEEEEEEEEEEEEEEEE          104
#define IDR_FFFFFFFFFFFFFFFFFFFFFFFFFFFF 106
#define IDD_GGGGGGGGGGGGGGGGGGGGGGGGGGGGGG 107
#define IDD_HHHHHHHHHHHHHHHHHHHHHHHHHHHHH 108
#define IDS_IIIIIIIIIIIIIIIIIIIII       111
#define IDS_JJJJJJJJJJJJJJJJJJJJJJJJ    112
#define IDS_KKKKKKKKKKKKKKKKKKK         114
#define IDD_LLLLLLLLLLLLLLLL            115

I don't want to suggest anything that would block this patch going in, ( because I know people have been waiting for it for a long time) , but ultimately I wonder if people may suggest having different macro alignment strategies and options later on.

Maybe if AlignConsecutiveMacros was an enumeration rather than just a Boolean (even if its only had None,Aligned for now) it might allow someone to more easily add a new strategy later on without breaking compatibility.

Apart form that I think its great, and thank you for taking the mantle.

[enyquist]

Looks fine to me, although I confess I did not build and run it because I don't have the time to set up the environment again, took a few hours last time I built from scratch (side note, if there's an easy way to speed up llvm/clang compilation up I'd love to hear it :) ).
You didn't change the tests that I can see, so if those are still passing then it seems to be a fairly successful refactor. However I'm very much a clang newbie (just to be clear), whether this satisfies the original request for a refactor I cannot comment on.

Thanks for taking this over-- I started this because I really needed the feature, but eventually gave up because we didn't need it anymore after a while, and after chasing it on here for nearly a year it just wasn't worth the effort anymore.
However if it turns out that this refactor really was all that was needed needed to merge it, while being slightly annoyed that I gave up on what turned out to be the *actual* final change, I will still be very happy to finally have this in clang-format :)

[VelocityRa]

In D28462#1376710, @enyquist wrote:

Looks fine to me, although I confess I did not build and run it because I don't have the time to set up the environment again, took a few hours last time I built from scratch (side note, if there's an easy way to speed up llvm/clang compilation up I'd love to hear it :) ).

I selected to only build clang-format from within Visual Studio by setting it up as the "Start-up project" (I'm sure it's possible regardless of the build env.) and it only took about 10 minutes - maybe less - on a quad core. It doesn't use many LLVM/Clang components.

Thanks for taking this over-- I started this because I really needed the feature, but eventually gave up because we didn't need it anymore after a while, and after chasing it on here for nearly a year it just wasn't worth the effort anymore.

Props to you for sticking around so long, I know how tedious these things can get.

[VelocityRa]

Ping.

[jacob-keller]

Is there anything holding this up that could use help?

[MyDeveloperDay]

@djasper and @klimek you both look relatively inactive in the last couple of weeks/month, is there anyone else who acts as a gate keeper for clang-format or can anyone add a LGTM?

[MyDeveloperDay]

I'm not a code owner here but we seem to be lacking people who are either available, able, willing or allowed to approve code reviews for some of the code in the clang-tooling (not just clang format). The code owners do an extremely good job at doing a deep review way beyond what many of the rest of us are able to do.. They've been doing this for years having written may of these tools in the first place, but I suspect other external pressures and their phabricator activity suggests they have limited time to contribute at present.

As such code reviews for new features seem to sit unreviewed and unaccepted and yet the subscribers list and tokens, the pings and "whats blocking this" comments would suggest that revisions such as these are wanted but sit for weeks and months with no movement.

It feels like we need more people reviewing code and approving code changes otherwise these revisions bit rot or simply do not get into the tool. It definitely will disenfranchise new developers from contributing, if it takes months/years to get anything committed. But a shallow review by someone with at least a little experience of this code is surely better than no review at all. As such I'd like to volunteer to help review some of these items.

Not all areas of clang have this same problem, I see areas of code which sometimes don't even get a review, where buddies review each others code very quickly, or where the time from creation to commit can be measured in hours or days, not weeks or months. I'm unclear what makes commits here take so long and good improvements lost. As such I don't think we get people contributing to fixing bugs either because the time taken to get even a bug landed seems quite great.

From what I understand as long as changes meet the guidelines set out about adding new options, changes have tests and have proper documentation then we should be free to approve and land those changes, The code owners have been given time like the rest of us to review and in the event they don't like the commit after the fact are free to submit a revision later or ask for further modifications. (make sure you check the done message on each of their review comments as you address them and ensure you leave a comment if you are not going to do the suggested change and why)

As developers if we submit a revision we need to be prepared to look after that change and address and defects that come in, we don't want people driving by and dropping a patch that the code owners have to maintain, so with committing come responsibility.

I've personally been running with this patch in my clang format since @VelocityRa took it over, and from what I can tell it works well, I too would like to see this landed.

With that said @VelocityRa and this comment, I would ask that you add something to the docs/ReleaseNotes.rst to show the addition of the new option to clang-format, and if you make that change, rebase (I doubt anything else was accepted in the meantime!) and send an updated revision I will add a LGTM and accept the review (for what that is worth), this will give @djasper and @klimek time to object to my proposal here.

Assuming others are in agreement, lets do something to move at least this forward.

[klimek]

In D28462#1405360, @MyDeveloperDay wrote:

I'm not a code owner here but we seem to be lacking people who are either available, able, willing or allowed to approve code reviews for some of the code in the clang-tooling (not just clang format). The code owners do an extremely good job at doing a deep review way beyond what many of the rest of us are able to do.. They've been doing this for years having written may of these tools in the first place, but I suspect other external pressures and their phabricator activity suggests they have limited time to contribute at present.

As such code reviews for new features seem to sit unreviewed and unaccepted and yet the subscribers list and tokens, the pings and "whats blocking this" comments would suggest that revisions such as these are wanted but sit for weeks and months with no movement.

It feels like we need more people reviewing code and approving code changes otherwise these revisions bit rot or simply do not get into the tool. It definitely will disenfranchise new developers from contributing, if it takes months/years to get anything committed. But a shallow review by someone with at least a little experience of this code is surely better than no review at all. As such I'd like to volunteer to help review some of these items.

I don't think shallow reviews before having the architectural questions tied down would help a lot, as they in my experience create more opposition of change later on.

I fully agree that the state we're in is bad, and I'm truly sorry for us to have arrived here.

Not all areas of clang have this same problem, I see areas of code which sometimes don't even get a review, where buddies review each others code very quickly, or where the time from creation to commit can be measured in hours or days, not weeks or months. I'm unclear what makes commits here take so long and good improvements lost. As such I don't think we get people contributing to fixing bugs either because the time taken to get even a bug landed seems quite great.

One of the differences is whether there is ahead-of-code-review-time agreement on the direction, and whether the code fits in well from an architectural point of view.
In clang-format we specifically do not want to support all possible features, but carefully weigh the upside of features against maintenance cost. Given that formatting is a prototypical bikeshed situation, I know that this is often subjective and thus really frustrating, and again, I'm truly sorry for it, but I don't have a good solution :(

From what I understand as long as changes meet the guidelines set out about adding new options, changes have tests and have proper documentation then we should be free to approve and land those changes, The code owners have been given time like the rest of us to review and in the event they don't like the commit after the fact are free to submit a revision later or ask for further modifications. (make sure you check the done message on each of their review comments as you address them and ensure you leave a comment if you are not going to do the suggested change and why)

I think there's consensus between folks responsible for clang-format that we do specifically not want all features. The question is not only whether the code follows guidelines and is documented and tested. All code incurs cost and that has to be weighted carefully against the benefits.

As developers if we submit a revision we need to be prepared to look after that change and address and defects that come in, we don't want people driving by and dropping a patch that the code owners have to maintain, so with committing come responsibility.

I've personally been running with this patch in my clang format since @VelocityRa took it over, and from what I can tell it works well, I too would like to see this landed.

With that said @VelocityRa and this comment, I would ask that you add something to the docs/ReleaseNotes.rst to show the addition of the new option to clang-format, and if you make that change, rebase (I doubt anything else was accepted in the meantime!) and send an updated revision I will add a LGTM and accept the review (for what that is worth), this will give @djasper and @klimek time to object to my proposal here.

Assuming others are in agreement, lets do something to move at least this forward.

[MyDeveloperDay]

In D28462#1405711, @klimek wrote:

In D28462#1405360, @MyDeveloperDay wrote:

I'm not a code owner here but we seem to be lacking people who are either available, able, willing or allowed to approve code reviews for some of the code in the clang-tooling (not just clang format). The code owners do an extremely good job at doing a deep review way beyond what many of the rest of us are able to do.. They've been doing this for years having written may of these tools in the first place, but I suspect other external pressures and their phabricator activity suggests they have limited time to contribute at present.

As such code reviews for new features seem to sit unreviewed and unaccepted and yet the subscribers list and tokens, the pings and "whats blocking this" comments would suggest that revisions such as these are wanted but sit for weeks and months with no movement.

It feels like we need more people reviewing code and approving code changes otherwise these revisions bit rot or simply do not get into the tool. It definitely will disenfranchise new developers from contributing, if it takes months/years to get anything committed. But a shallow review by someone with at least a little experience of this code is surely better than no review at all. As such I'd like to volunteer to help review some of these items.

I don't think shallow reviews before having the architectural questions tied down would help a lot, as they in my experience create more opposition of change later on.

I fully agree that the state we're in is bad, and I'm truly sorry for us to have arrived here.

Not all areas of clang have this same problem, I see areas of code which sometimes don't even get a review, where buddies review each others code very quickly, or where the time from creation to commit can be measured in hours or days, not weeks or months. I'm unclear what makes commits here take so long and good improvements lost. As such I don't think we get people contributing to fixing bugs either because the time taken to get even a bug landed seems quite great.

One of the differences is whether there is ahead-of-code-review-time agreement on the direction, and whether the code fits in well from an architectural point of view.
In clang-format we specifically do not want to support all possible features, but carefully weigh the upside of features against maintenance cost. Given that formatting is a prototypical bikeshed situation, I know that this is often subjective and thus really frustrating, and again, I'm truly sorry for it, but I don't have a good solution :(

From what I understand as long as changes meet the guidelines set out about adding new options, changes have tests and have proper documentation then we should be free to approve and land those changes, The code owners have been given time like the rest of us to review and in the event they don't like the commit after the fact are free to submit a revision later or ask for further modifications. (make sure you check the done message on each of their review comments as you address them and ensure you leave a comment if you are not going to do the suggested change and why)

I think there's consensus between folks responsible for clang-format that we do specifically not want all features. The question is not only whether the code follows guidelines and is documented and tested. All code incurs cost and that has to be weighted carefully against the benefits.

As developers if we submit a revision we need to be prepared to look after that change and address and defects that come in, we don't want people driving by and dropping a patch that the code owners have to maintain, so with committing come responsibility.

I've personally been running with this patch in my clang format since @VelocityRa took it over, and from what I can tell it works well, I too would like to see this landed.

With that said @VelocityRa and this comment, I would ask that you add something to the docs/ReleaseNotes.rst to show the addition of the new option to clang-format, and if you make that change, rebase (I doubt anything else was accepted in the meantime!) and send an updated revision I will add a LGTM and accept the review (for what that is worth), this will give @djasper and @klimek time to object to my proposal here.

Assuming others are in agreement, lets do something to move at least this forward.

Thank you for responsing.. It seems no one is is able to approve code here without your or @djasper's input, of course we understand that is the ideal situation because you wrote this in the first place, and your ability to understand the code is far superior than others. But i'm sure you time is precious, how can the rest of us grow to help out?

I've heard the mantra before about "the cost of development", I'm not sure I understand why the "unit tests" aren't enough to prevent new styles breaking old capability, it feels like we don't have confidence that adding more won't break what is there already, and that is a lack of confidence that normally comes when code doesn't have enough tests, so why is clang-format so special that we can't cope with future additions? when the rest of clang (& C++ for that matter) is changing so rapidly underneath?

To learn a code base in order to be able to help and spread that development cost, you need to work in that code base, that means adding things, fixing things, reviewing things. approving things... are you happy for others to contribute?

[klimek]

In D28462#1405855, @MyDeveloperDay wrote:

In D28462#1405711, @klimek wrote:

In D28462#1405360, @MyDeveloperDay wrote:

I'm not a code owner here but we seem to be lacking people who are either available, able, willing or allowed to approve code reviews for some of the code in the clang-tooling (not just clang format). The code owners do an extremely good job at doing a deep review way beyond what many of the rest of us are able to do.. They've been doing this for years having written may of these tools in the first place, but I suspect other external pressures and their phabricator activity suggests they have limited time to contribute at present.

As such code reviews for new features seem to sit unreviewed and unaccepted and yet the subscribers list and tokens, the pings and "whats blocking this" comments would suggest that revisions such as these are wanted but sit for weeks and months with no movement.

It feels like we need more people reviewing code and approving code changes otherwise these revisions bit rot or simply do not get into the tool. It definitely will disenfranchise new developers from contributing, if it takes months/years to get anything committed. But a shallow review by someone with at least a little experience of this code is surely better than no review at all. As such I'd like to volunteer to help review some of these items.

I don't think shallow reviews before having the architectural questions tied down would help a lot, as they in my experience create more opposition of change later on.

I fully agree that the state we're in is bad, and I'm truly sorry for us to have arrived here.

Not all areas of clang have this same problem, I see areas of code which sometimes don't even get a review, where buddies review each others code very quickly, or where the time from creation to commit can be measured in hours or days, not weeks or months. I'm unclear what makes commits here take so long and good improvements lost. As such I don't think we get people contributing to fixing bugs either because the time taken to get even a bug landed seems quite great.

One of the differences is whether there is ahead-of-code-review-time agreement on the direction, and whether the code fits in well from an architectural point of view.
In clang-format we specifically do not want to support all possible features, but carefully weigh the upside of features against maintenance cost. Given that formatting is a prototypical bikeshed situation, I know that this is often subjective and thus really frustrating, and again, I'm truly sorry for it, but I don't have a good solution :(

From what I understand as long as changes meet the guidelines set out about adding new options, changes have tests and have proper documentation then we should be free to approve and land those changes, The code owners have been given time like the rest of us to review and in the event they don't like the commit after the fact are free to submit a revision later or ask for further modifications. (make sure you check the done message on each of their review comments as you address them and ensure you leave a comment if you are not going to do the suggested change and why)

I think there's consensus between folks responsible for clang-format that we do specifically not want all features. The question is not only whether the code follows guidelines and is documented and tested. All code incurs cost and that has to be weighted carefully against the benefits.

As developers if we submit a revision we need to be prepared to look after that change and address and defects that come in, we don't want people driving by and dropping a patch that the code owners have to maintain, so with committing come responsibility.

I've personally been running with this patch in my clang format since @VelocityRa took it over, and from what I can tell it works well, I too would like to see this landed.

With that said @VelocityRa and this comment, I would ask that you add something to the docs/ReleaseNotes.rst to show the addition of the new option to clang-format, and if you make that change, rebase (I doubt anything else was accepted in the meantime!) and send an updated revision I will add a LGTM and accept the review (for what that is worth), this will give @djasper and @klimek time to object to my proposal here.

Assuming others are in agreement, lets do something to move at least this forward.

Thank you for responsing.. It seems no one is is able to approve code here without your or @djasper's input, of course we understand that is the ideal situation because you wrote this in the first place, and your ability to understand the code is far superior than others. But i'm sure you time is precious, how can the rest of us grow to help out?

I've heard the mantra before about "the cost of development", I'm not sure I understand why the "unit tests" aren't enough to prevent new styles breaking old capability, it feels like we don't have confidence that adding more won't break what is there already, and that is a lack of confidence that normally comes when code doesn't have enough tests, so why is clang-format so special that we can't cope with future additions? when the rest of clang (& C++ for that matter) is changing so rapidly underneath?

I don't think it's particularly different? The main difference is that it's much easier to come up with a change to clang-format that you really really want than to clang, because clang has a standard and clang-format invites contributions that are a matter of taste, which quickly leads to very heated arguments over things that one side might consider diminishing returns.

To learn a code base in order to be able to help and spread that development cost, you need to work in that code base, that means adding things, fixing things, reviewing things. approving things... are you happy for others to contribute?

Yes, we are, and as you said, it needs a set of patches to learn the code base enough to contribute, to build up trust. This is easiest when the code base is currently under active development, but many people consider clang-format "good enough" minus bug fixes / new languages (like the C# patch, which I haven't looked at).

Are you by chance going to be at the Euro LLVM dev meeting? If so, I'd be happy to sit together with you to work on / talk through patches.

Overall, note that I don't see big architectural problems with this patch so far - it took me a while to realize it was re-written, but I believe architecturally it is fine and needs a good pass on the technical details before going in.

[MyDeveloperDay]

Yes, we are, and as you said, it needs a set of patches to learn the code base enough to contribute, to build up trust. This is easiest when the code base is currently under active development, but many people consider clang-format "good enough" minus bug fixes / new languages (like the C# patch, which I haven't looked at).

Are you by chance going to be at the Euro LLVM dev meeting? If so, I'd be happy to sit together with you to work on / talk through patches.

Alas I am not. I would however value your input on the C# patch, as I work running it through some larger C# code bases I've found some constructs that need additional changes. In the meantime my proposal still stands, I'd like to help, if that just means doing the leg work of ensuring patches have documentation, are formatted etc... but I don't want to railroad this revision with that discussion.

[jacob-keller]

In regards to whether clang-format should support this feature, I think it's pretty clear that a large number of users want it to. The tool already supports formatting various other definitions in a similar manner, including variables and struct members. So I don't see how a similar features which aligns consecutive macros is something which doesn't make sense.

Additionally, because the formatting will *undo* such formatting if done manually, it means that the existing source code formatting this way cannot be handled via clang-format. In my case, it essentially means that I cannot use clang-format to enforce the style guidelines, as the macros definitions are aligned.

Additionally, aligning a series of macros makes sense because it helps improve readability, and is thus something that I'd rather not lose if I were to switch to using clang-format without the feature.

[micah-s]

ClamAV recently started using clang-format. We published this blog post about how we're using it: https://blog.clamav.net/2019/02/clamav-adopts-clang-format.html One of the things I called out in the blog post is that we really want this feature, and presently we have to use the "// clang-format on/off" markers throughout our code mostly to keep our consecutive macros aligned.

I haven't found time to build clang-format with this patch to test it on our code base with the markers disabled. If a review of my experience doing so will help get traction for this review in any way, let me know and I'll make the effort.

[MyDeveloperDay]

In D28462#1407239, @micah-s wrote:

ClamAV recently started using clang-format. We published this blog post about how we're using it: https://blog.clamav.net/2019/02/clamav-adopts-clang-format.html One of the things I called out in the blog post is that we really want this feature, and presently we have to use the "// clang-format on/off" markers throughout our code mostly to keep our consecutive macros aligned.

I haven't found time to build clang-format with this patch to test it on our code base with the markers disabled. If a review of my experience doing so will help get traction for this review in any way, let me know and I'll make the effort.

For those wishing to test the effectiveness of unlanded revisions like this and to reduce the amount of time between Windows snapshot builds (https://llvm.org/builds/), I have forked llvm-project and using AppVeyor to build a new x64 Windows clang-format-experimental.exe binary, on a semi-automatic basis. (master branch clang-format with selected unlanded revisions)

https://github.com/mydeveloperday/clang-experimental/releases

When a revision like this meets excessive delay in the review cycle, feel free to log a github issue, Early testing of unlanded features may help to prove the usefulness of a revision, provide useful review comments and/or help find bugs earlier in the development cycle.

[micah-s]

In D28462#1412556, @MyDeveloperDay wrote:

For those wishing to test the effectiveness of unlanded revisions like this and to reduce the amount of time between Windows snapshot builds (https://llvm.org/builds/), I have forked llvm-project and using AppVeyor to build a new x64 Windows clang-format-experimental.exe binary, on a semi-automatic basis. (master branch clang-format with selected unlanded revisions)

https://github.com/mydeveloperday/clang-experimental/releases

Thanks @MyDeveloperDay! That is very helpful, and significantly reduced the overhead for testing.

I removed the // clang-format off/on markers around all of our aligned macros, applied the AlignConsecurityMacros: true setting to our .clang-format, and reformatted the affected files using the clang-format-experimental.exe binary. My own experience was flawless. The only issue we'll have to work around is that we have some longer macro sets that include unused values in a series.

For example:

...
#define CL_SCAN_ALGORITHMIC       0x200
//#define UNUSED                  0x400
#define CL_SCAN_PHISHING_BLOCKSSL 0x800
...

becomes

...
#define CL_SCAN_ALGORITHMIC 0x200
//#define UNUSED                  0x400
#define CL_SCAN_PHISHING_BLOCKSSL 0x800
...

I can't really complain though, because it's working as intended -- we'll just have to find a different way to indicate reserved/unused values in a series. I'll probably just do something like CL_SCAN_UNUSED_0/1/2/etc.
Big thumbs up here from an end user experience.

[MyDeveloperDay]

ClamAV recently started using clang-format. We published this blog post about how we're using it: https://blog.clamav.net/2019/02/clamav-adopts-clang-format.html One of the things I called out in the blog post is that we really want this feature, and presently we have to use the "// clang-format on/off" markers throughout our code mostly to keep our consecutive macros aligned.

Would you please consider logging your struct example from your blog at https://bugs.llvm.org, I see a large number of people using clang format on/off in github for this and similar issues (including google code!), looks like clang-format needs some struct breaking rules

[micah-s]

In D28462#1413332, @MyDeveloperDay wrote:

Would you please consider logging your struct example from your blog at https://bugs.llvm.org, I see a large number of people using clang format on/off in github for this and similar issues (including google code!), looks like clang-format needs some struct breaking rules

Will do!

[klimek]

In D28462#1406716, @jacob-keller wrote:

In regards to whether clang-format should support this feature, I think it's pretty clear that a large number of users want it to. The tool already supports formatting various other definitions in a similar manner, including variables and struct members. So I don't see how a similar features which aligns consecutive macros is something which doesn't make sense.

Additionally, because the formatting will *undo* such formatting if done manually, it means that the existing source code formatting this way cannot be handled via clang-format. In my case, it essentially means that I cannot use clang-format to enforce the style guidelines, as the macros definitions are aligned.

Additionally, aligning a series of macros makes sense because it helps improve readability, and is thus something that I'd rather not lose if I were to switch to using clang-format without the feature.

lib/Format/WhitespaceManager.cpp

436 ↗

(On Diff #184010)

I don't think the 'All' postfix in the name is helpful. What are you trying to say with that name?

437 ↗

(On Diff #184010)

Why an rvalue ref? Also, this is used only once, so why make this a template?

442 ↗

(On Diff #184010)

llvm style: use an upper case I for index vars.

473 ↗

(On Diff #184010)

These are indices into Matches, not line numbers, right?

497 ↗

(On Diff #184010)

llvm style: use upper-case I and E.

500 ↗

(On Diff #184010)

Why set EndOfSequence outside the if below?

512 ↗

(On Diff #184010)

What's the reason for this?

[VelocityRa]

Waiting for further feedback before pushing an update.

lib/Format/WhitespaceManager.cpp

436 ↗

(On Diff #184010)

I'm not particularly fond of All either, suggestions welcome.

As the comment above explains, All refers to the fact that it operates on all tokens, instead of being limited to certain cases like AlignTokenSequence is. Maybe I should name this one AlignTokenSequence and the other one AlignTokenSequenceOuterScope, or something.

437 ↗

(On Diff #184010)

It's an rvalue ref, because that's also the case for AlignTokenSequence (wasn't sure why, so I left it as is).
It's used only once, but the function is more generic that way, no? That's the point of its generic name.
Tell me if I should change it.

442 ↗

(On Diff #184010)

Ok, I assume your style changed because this is copied from AlignTokenSequence.

473 ↗

(On Diff #184010)

Correct. My bad.

500 ↗

(On Diff #184010)

It's from AlignTokens. I think it's because due to some of the loop logic, it ends up not checking up to the point of the the last token.
Without setting this and calling AlignCurrentSequence() once more at the end, the last line of a macro group does not get properly aligned, the tests fail.

512 ↗

(On Diff #184010)

I don't remember, but it was unnecessary apparently. The tests pass without this check.

[klimek]

lib/Format/WhitespaceManager.cpp

436 ↗

(On Diff #184010)

How about calling this AlignMacroSequence and the other one AlignMacros.
Comment here would be something like (as I don't think "scope" plays a role??)
// Aligns a sequence of macro definitions.

The problem is that I think the alignTokensAll don't make sense either as a function.
We should be able to inline this into alignConsecutiveMacros, and pull the std::function handed in either into its own function, or just define it as the first step in alignConsecutiveMacros.

437 ↗

(On Diff #184010)

I think we need to look at this from first principles. We can fix the old code later if it doesn't make sense :)

442 ↗

(On Diff #184010)

Yea, sorry, those are in violation (we should fix that at some point, but *not* in this patch :)

500 ↗

(On Diff #184010)

I was suggesting to move it inside the if below, did you try that (sounds like you tried to remove it).

[VelocityRa]

Diff updated.
Some comments about the inline changes, since the rest were trivial:

• AlignMacroSequence was inlined
• AlignMacros was inlined after that
• AlignMacrosMatches can't be inlined because it's used both by alignConsecutiveMacros's (pre-AlignMacros) AlignCurrentSequence lambda and alignConsecutiveMacros itself.
• AlignCurrentSequence can't be inlined because it's used three times in alignConsecutiveMacros.

lib/Format/WhitespaceManager.cpp

500 ↗

(On Diff #184010)

I moved it so that the below if is:

if (Changes[i].NewlinesBefore > 1 || !FoundMatchOnLine) {
  EndOfSequence = i;
  AlignCurrentSequence();
}

and it did not work correctly.

[klimek]

lib/Format/WhitespaceManager.cpp

482 ↗

(On Diff #191493)

Ok, sorry, but you went one step further here than I imagined :)
The idea would be to unlinline this (and only this :D) and call it AligneMacroSequence.

[VelocityRa]

Right, how's that?

[klimek]

lib/Format/WhitespaceManager.cpp

520 ↗

(On Diff #191891)

Why are we calling AlignMacroSequence(0, 0, ...) here?

524 ↗

(On Diff #191891)

I still think the code in this loop either needs significantly more comments to explain what's going on, or needs to be changed to be more straight forward:
What I don't understand is why we're calling AlignMacroSequence potentially multiple times, and especially what things like the if in line 541 are for.

[VelocityRa]

lib/Format/WhitespaceManager.cpp

520 ↗

(On Diff #191891)

Wasn't needed apparently.

524 ↗

(On Diff #191891)

That if and surrounding logic also exists and is copied from AlignTokens and it's not documented there.
Anyway, apparently the extra AlignMacroSequence in this loop could be removed. I tried iteratively removing/tweaking other snippets like the if and tests failed.

[jkorous]

Hi @VelocityRa, just FYI - it's considered fine to ping your reviewers once per week here if you've addressed their comments and there's no activity in the review. Sometimes people just get distracted by other things.

[micah-s]

@djasper @klimek @krasimir @sammccall @enyquist Can I trouble you for an update?

In D28462#1492600, @jkorous wrote:

Hi @VelocityRa, just FYI - it's considered fine to ping your reviewers once per week here if you've addressed their comments and there's no activity in the review. Sometimes people just get distracted by other things.

[MyDeveloperDay]

When I spoke with one of the code owners, they seemed to have a problem accepting this review based on there not being a general description/understanding of how this algorithm works.

Having said that, all of the "AlignToken" based functions (alignConsecutiveXXXX) are hard to understand unless you take the AlignTokens() function as read, and just look at the Lambda.

As a relative new comer, I find some areas of clang-format hard to understand, I tend to learn them as and when I need to, but the gist of AlignTokens is in its comment, but with comments like 'There is a non-obvious subtlety' probably means you really need to be in it and inside a debugger to see what is going on.

For this review I actually don't personally see what is Macro specific about AlignMacroSequence(), it seems to be almost identical to AlignTokenSequence without the scope and comment handling, (which actually makes it a lot simpler), my feeling is it could be used for more than just this, maybe pulling this out into its own set of functions wasn't the correct thing to do, and actually the original design might have been better, but this solution seems more simplistic and because its isolated means it doesn't break the other functions.

All I feel I can add to the review process, is that perhaps a few more comments explaining the subtlety of the various "if statements" in both the Lambda and in the main alignment might help bring some clarify.. but to be honest I'm OK with how it works and I'd like it in.

Ultimately I'd also like to see this revision land, especially as its off by default, I would like to see some ability to be able to set a Minimum Column distance to the value (this would allow Visual Studio to not keep playing the hokey-cokey (https://en.wikipedia.org/wiki/Hokey_cokey) with resource.h files.), but perhaps that is for a later change once this is accepted.

From my perspective it LGTM, if the code owners are present they should really rubber stamp it, If not well...

[MyDeveloperDay]

@klimek responded via the mailing list

http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20190506/270976.html

[phosek]

Would it be possible to land this change? Do you need help with landing it? We've been waiting for this for a long time and we would like to start using it now that it has been accepted.

[vit9696]

Could somebody commit it into 9.0? Quite a lot of people depend on this option including us. Since it got accepted I do not see a reason for this to get postponed any further. Thanks!

[• LingDev]

We need this feature as well, we are also waiting for it.

[sammccall]

It sounds like this is fairly low-risk (it's behind a flag that's off by default), has been approved, and we want it in clang-format 9, so I'm going to rebase and land it.

I don't understand the patch in detail so if there is significant fallout I'll have to revert and leave to @MyDeveloperDay and @VelocityRa.

[sylvestre.ledru]

@VelocityRa could you please add it to the release notes?
https://github.com/llvm-mirror/clang/blob/master/docs/ReleaseNotes.rst
I can commit the change if you need

[VelocityRa]

In D28462#1575262, @sylvestre.ledru wrote:

@VelocityRa could you please add it to the release notes?
https://github.com/llvm-mirror/clang/blob/master/docs/ReleaseNotes.rst
I can commit the change if you need

Yeah could you do it? Thank you.

[sylvestre.ledru]

Sure, done in r365445
thanks for your work!

cfe/trunk/docs/ClangFormatStyleOptions.rst

* ``BAS_AlwaysBreak`` (in configuration: ``AlwaysBreak``)
 
     .. code-block:: c++
 
       someLongFunction(
           argument1, argument2);
 
 
 
+**AlignConsecutiveMacros** (``bool``)
+  If ``true``, aligns consecutive C/C++ preprocessor macros.
+
+  This will align the C/C++ preprocessor macros of consecutive lines. This
+  will result in formattings like
+
+  .. code-block:: c++
+
+    #define SHORT_NAME       42
+    #define LONGER_NAME      0x007f
+    #define EVEN_LONGER_NAME (2)
+    #define foo(x)           (x * x)
+    #define bar(y, z)        (y + z)
+
 **AlignConsecutiveAssignments** (``bool``)
   If ``true``, aligns consecutive assignments.
 
   This will align the assignment operators of consecutive lines. This
   will result in formattings like
 
   .. code-block:: c++
 

cfe/trunk/include/clang/Format/Format.h

struct FormatStyle {
   };
 
   /// If ``true``, horizontally aligns arguments after an open bracket.
   ///
   /// This applies to round brackets (parentheses), angle brackets and square
   /// brackets.
   BracketAlignmentStyle AlignAfterOpenBracket;
 
+  /// \brief If ``true``, aligns consecutive C/C++ preprocessor macros.
+  ///
+  /// This will align C/C++ preprocessor macros of consecutive lines.
+  /// Will result in formattings like
+  /// \code
+  ///   #define SHORT_NAME       42
+  ///   #define LONGER_NAME      0x007f
+  ///   #define EVEN_LONGER_NAME (2)
+  ///   #define foo(x)           (x * x)
+  ///   #define bar(y, z)        (y + z)
+  /// \endcode
+  bool AlignConsecutiveMacros;
+
   /// If ``true``, aligns consecutive assignments.
   ///
   /// This will align the assignment operators of consecutive lines. This
   /// will result in formattings like
   /// \code
   ///   int aaaa = 12;
   ///   int b    = 23;
   ///   int ccc  = 23;

cfe/trunk/lib/Format/Format.cpp

if (!IO.outputting()) {
                      Style.IndentWrappedFunctionNames);
       IO.mapOptional("PointerBindsToType", Style.PointerAlignment);
       IO.mapOptional("SpaceAfterControlStatementKeyword",
                      Style.SpaceBeforeParens);
     }
 
     IO.mapOptional("AccessModifierOffset", Style.AccessModifierOffset);
     IO.mapOptional("AlignAfterOpenBracket", Style.AlignAfterOpenBracket);
+    IO.mapOptional("AlignConsecutiveMacros", Style.AlignConsecutiveMacros);
     IO.mapOptional("AlignConsecutiveAssignments",
                    Style.AlignConsecutiveAssignments);
     IO.mapOptional("AlignConsecutiveDeclarations",
                    Style.AlignConsecutiveDeclarations);
     IO.mapOptional("AlignEscapedNewlines", Style.AlignEscapedNewlines);
     IO.mapOptional("AlignOperands", Style.AlignOperands);
     IO.mapOptional("AlignTrailingComments", Style.AlignTrailingComments);
     IO.mapOptional("AllowAllArgumentsOnNextLine",
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.Language = Language;
   LLVMStyle.AccessModifierOffset = -2;
   LLVMStyle.AlignEscapedNewlines = FormatStyle::ENAS_Right;
   LLVMStyle.AlignAfterOpenBracket = FormatStyle::BAS_Align;
   LLVMStyle.AlignOperands = true;
   LLVMStyle.AlignTrailingComments = true;
   LLVMStyle.AlignConsecutiveAssignments = false;
   LLVMStyle.AlignConsecutiveDeclarations = false;
+  LLVMStyle.AlignConsecutiveMacros = false;
   LLVMStyle.AllowAllArgumentsOnNextLine = true;
   LLVMStyle.AllowAllConstructorInitializersOnNextLine = true;
   LLVMStyle.AllowAllParametersOfDeclarationOnNextLine = true;
   LLVMStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
   LLVMStyle.AllowShortBlocksOnASingleLine = false;
   LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
   LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   LLVMStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;

cfe/trunk/lib/Format/WhitespaceManager.h

public:
   };
 
 private:
   /// Calculate \c IsTrailingComment, \c TokenLength for the last tokens
   /// or token parts in a line and \c PreviousEndOfTokenColumn and
   /// \c EscapedNewlineColumn for the first tokens or token parts in a line.
   void calculateLineBreakInformation();
 
+  /// \brief Align consecutive C/C++ preprocessor macros over all \c Changes.
+  void alignConsecutiveMacros();
+
   /// Align consecutive assignments over all \c Changes.
   void alignConsecutiveAssignments();
 
   /// Align consecutive declarations over all \c Changes.
   void alignConsecutiveDeclarations();
 
   /// Align trailing comments over all \c Changes.
   void alignTrailingComments();

cfe/trunk/lib/Format/WhitespaceManager.cpp

 }
 
 const tooling::Replacements &WhitespaceManager::generateReplacements() {
   if (Changes.empty())
     return Replaces;
 
   llvm::sort(Changes, Change::IsBeforeInFile(SourceMgr));
   calculateLineBreakInformation();
+  alignConsecutiveMacros();
   alignConsecutiveDeclarations();
   alignConsecutiveAssignments();
   alignTrailingComments();
   alignEscapedNewlines();
   generateChanges();
 
   return Replaces;
 }
for (unsigned e = Changes.size(); i != e; ++i) {
     MaxColumn = std::min(MaxColumn, ChangeMaxColumn);
   }
 
   EndOfSequence = i;
   AlignCurrentSequence();
   return i;
 }
 
+// Aligns a sequence of matching tokens, on the MinColumn column.
+//
+// Sequences start from the first matching token to align, and end at the
+// first token of the first line that doesn't need to be aligned.
+//
+// We need to adjust the StartOfTokenColumn of each Change that is on a line
+// containing any matching token to be aligned and located after such token.
+static void AlignMacroSequence(
+    unsigned &StartOfSequence, unsigned &EndOfSequence, unsigned &MinColumn,
+    unsigned &MaxColumn, bool &FoundMatchOnLine,
+    std::function<bool(const WhitespaceManager::Change &C)> AlignMacrosMatches,
+    SmallVector<WhitespaceManager::Change, 16> &Changes) {
+  if (StartOfSequence > 0 && StartOfSequence < EndOfSequence) {
+
+    FoundMatchOnLine = false;
+    int Shift = 0;
+
+    for (unsigned I = StartOfSequence; I != EndOfSequence; ++I) {
+      if (Changes[I].NewlinesBefore > 0) {
+        Shift = 0;
+        FoundMatchOnLine = false;
+      }
+
+      // If this is the first matching token to be aligned, remember by how many
+      // spaces it has to be shifted, so the rest of the changes on the line are
+      // shifted by the same amount
+      if (!FoundMatchOnLine && AlignMacrosMatches(Changes[I])) {
+        FoundMatchOnLine = true;
+        Shift = MinColumn - Changes[I].StartOfTokenColumn;
+        Changes[I].Spaces += Shift;
+      }
+
+      assert(Shift >= 0);
+      Changes[I].StartOfTokenColumn += Shift;
+      if (I + 1 != Changes.size())
+        Changes[I + 1].PreviousEndOfTokenColumn += Shift;
+    }
+  }
+
+  MinColumn = 0;
+  MaxColumn = UINT_MAX;
+  StartOfSequence = 0;
+  EndOfSequence = 0;
+}
+
+void WhitespaceManager::alignConsecutiveMacros() {
+  if (!Style.AlignConsecutiveMacros)
+    return;
+
+  auto AlignMacrosMatches = [](const Change &C) {
+    const FormatToken *Current = C.Tok;
+    unsigned SpacesRequiredBefore = 1;
+
+    if (Current->SpacesRequiredBefore == 0 || !Current->Previous)
+      return false;
+
+    Current = Current->Previous;
+
+    // If token is a ")", skip over the parameter list, to the
+    // token that precedes the "("
+    if (Current->is(tok::r_paren) && Current->MatchingParen) {
+      Current = Current->MatchingParen->Previous;
+      SpacesRequiredBefore = 0;
+    }
+
+    if (!Current || !Current->is(tok::identifier))
+      return false;
+
+    if (!Current->Previous || !Current->Previous->is(tok::pp_define))
+      return false;
+
+    // For a macro function, 0 spaces are required between the
+    // identifier and the lparen that opens the parameter list.
+    // For a simple macro, 1 space is required between the
+    // identifier and the first token of the defined value.
+    return Current->Next->SpacesRequiredBefore == SpacesRequiredBefore;
+  };
+
+  unsigned MinColumn = 0;
+  unsigned MaxColumn = UINT_MAX;
+
+  // Start and end of the token sequence we're processing.
+  unsigned StartOfSequence = 0;
+  unsigned EndOfSequence = 0;
+
+  // Whether a matching token has been found on the current line.
+  bool FoundMatchOnLine = false;
+
+  unsigned I = 0;
+  for (unsigned E = Changes.size(); I != E; ++I) {
+    if (Changes[I].NewlinesBefore != 0) {
+      EndOfSequence = I;
+      // If there is a blank line, or if the last line didn't contain any
+      // matching token, the sequence ends here.
+      if (Changes[I].NewlinesBefore > 1 || !FoundMatchOnLine)
+        AlignMacroSequence(StartOfSequence, EndOfSequence, MinColumn, MaxColumn,
+                           FoundMatchOnLine, AlignMacrosMatches, Changes);
+
+      FoundMatchOnLine = false;
+    }
+
+    if (!AlignMacrosMatches(Changes[I]))
+      continue;
+
+    FoundMatchOnLine = true;
+
+    if (StartOfSequence == 0)
+      StartOfSequence = I;
+
+    unsigned ChangeMinColumn = Changes[I].StartOfTokenColumn;
+    int LineLengthAfter = -Changes[I].Spaces;
+    for (unsigned j = I; j != E && Changes[j].NewlinesBefore == 0; ++j)
+      LineLengthAfter += Changes[j].Spaces + Changes[j].TokenLength;
+    unsigned ChangeMaxColumn = Style.ColumnLimit - LineLengthAfter;
+
+    MinColumn = std::max(MinColumn, ChangeMinColumn);
+    MaxColumn = std::min(MaxColumn, ChangeMaxColumn);
+  }
+
+  EndOfSequence = I;
+  AlignMacroSequence(StartOfSequence, EndOfSequence, MinColumn, MaxColumn,
+                     FoundMatchOnLine, AlignMacrosMatches, Changes);
+}
+
 void WhitespaceManager::alignConsecutiveAssignments() {
   if (!Style.AlignConsecutiveAssignments)
     return;
 
   AlignTokens(
       Style,
       [&](const Change &C) {
         // Do not align on equal signs that are first on a line.

cfe/trunk/unittests/Format/FormatTest.cpp

verifyFormat("{\n"
                NoSpaceStyle);
   verifyFormat("switch (x) {\n"
                "case 1:\n"
                "default:\n"
                "}",
                NoSpaceStyle);
 }
 
+TEST_F(FormatTest, AlignConsecutiveMacros) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AlignConsecutiveAssignments = true;
+  Style.AlignConsecutiveDeclarations = true;
+  Style.AlignConsecutiveMacros = false;
+
+  verifyFormat("#define a 3\n"
+               "#define bbbb 4\n"
+               "#define ccc (5)",
+               Style);
+
+  verifyFormat("#define f(x) (x * x)\n"
+               "#define fff(x, y, z) (x * y + z)\n"
+               "#define ffff(x, y) (x - y)",
+               Style);
+
+  verifyFormat("#define foo(x, y) (x + y)\n"
+               "#define bar (5, 6)(2 + 2)",
+               Style);
+
+  verifyFormat("#define a 3\n"
+               "#define bbbb 4\n"
+               "#define ccc (5)\n"
+               "#define f(x) (x * x)\n"
+               "#define fff(x, y, z) (x * y + z)\n"
+               "#define ffff(x, y) (x - y)",
+               Style);
+
+  Style.AlignConsecutiveMacros = true;
+  verifyFormat("#define a    3\n"
+               "#define bbbb 4\n"
+               "#define ccc  (5)",
+               Style);
+
+  verifyFormat("#define f(x)         (x * x)\n"
+               "#define fff(x, y, z) (x * y + z)\n"
+               "#define ffff(x, y)   (x - y)",
+               Style);
+
+  verifyFormat("#define foo(x, y) (x + y)\n"
+               "#define bar       (5, 6)(2 + 2)",
+               Style);
+
+  verifyFormat("#define a            3\n"
+               "#define bbbb         4\n"
+               "#define ccc          (5)\n"
+               "#define f(x)         (x * x)\n"
+               "#define fff(x, y, z) (x * y + z)\n"
+               "#define ffff(x, y)   (x - y)",
+               Style);
+
+  verifyFormat("#define a         5\n"
+               "#define foo(x, y) (x + y)\n"
+               "#define CCC       (6)\n"
+               "auto lambda = []() {\n"
+               "  auto  ii = 0;\n"
+               "  float j  = 0;\n"
+               "  return 0;\n"
+               "};\n"
+               "int   i  = 0;\n"
+               "float i2 = 0;\n"
+               "auto  v  = type{\n"
+               "    i = 1,   //\n"
+               "    (i = 2), //\n"
+               "    i = 3    //\n"
+               "};",
+               Style);
+
+  Style.AlignConsecutiveMacros = false;
+  Style.ColumnLimit = 20;
+
+  verifyFormat("#define a          \\\n"
+               "  \"aabbbbbbbbbbbb\"\n"
+               "#define D          \\\n"
+               "  \"aabbbbbbbbbbbb\" \\\n"
+               "  \"ccddeeeeeeeee\"\n"
+               "#define B          \\\n"
+               "  \"QQQQQQQQQQQQQ\"  \\\n"
+               "  \"FFFFFFFFFFFFF\"  \\\n"
+               "  \"LLLLLLLL\"\n",
+               Style);
+
+  Style.AlignConsecutiveMacros = true;
+  verifyFormat("#define a          \\\n"
+               "  \"aabbbbbbbbbbbb\"\n"
+               "#define D          \\\n"
+               "  \"aabbbbbbbbbbbb\" \\\n"
+               "  \"ccddeeeeeeeee\"\n"
+               "#define B          \\\n"
+               "  \"QQQQQQQQQQQQQ\"  \\\n"
+               "  \"FFFFFFFFFFFFF\"  \\\n"
+               "  \"LLLLLLLL\"\n",
+               Style);
+}
+
 TEST_F(FormatTest, AlignConsecutiveAssignments) {
   FormatStyle Alignment = getLLVMStyle();
+  Alignment.AlignConsecutiveMacros = true;
   Alignment.AlignConsecutiveAssignments = false;
   verifyFormat("int a = 5;\n"
                "int oneTwoThree = 123;",
                Alignment);
   verifyFormat("int a = 5;\n"
                "int oneTwoThree = 123;",
                Alignment);
 
TEST_F(FormatTest, AlignConsecutiveAssignments) {
   verifyFormat("for (i = 0; i < 1; i++)\n"
                "  x = 1;\n"
                "y = 1;\n",
                Alignment);
 }
 
 TEST_F(FormatTest, AlignConsecutiveDeclarations) {
   FormatStyle Alignment = getLLVMStyle();
+  Alignment.AlignConsecutiveMacros = true;
   Alignment.AlignConsecutiveDeclarations = false;
   verifyFormat("float const a = 5;\n"
                "int oneTwoThree = 123;",
                Alignment);
   verifyFormat("int a = 5;\n"
                "float const oneTwoThree = 123;",
                Alignment);
 

 
 TEST_F(FormatTest, ParsesConfigurationBools) {
   FormatStyle Style = {};
   Style.Language = FormatStyle::LK_Cpp;
   CHECK_PARSE_BOOL(AlignOperands);
   CHECK_PARSE_BOOL(AlignTrailingComments);
   CHECK_PARSE_BOOL(AlignConsecutiveAssignments);
   CHECK_PARSE_BOOL(AlignConsecutiveDeclarations);
+  CHECK_PARSE_BOOL(AlignConsecutiveMacros);
   CHECK_PARSE_BOOL(AllowAllArgumentsOnNextLine);
   CHECK_PARSE_BOOL(AllowAllConstructorInitializersOnNextLine);
   CHECK_PARSE_BOOL(AllowAllParametersOfDeclarationOnNextLine);
   CHECK_PARSE_BOOL(AllowShortBlocksOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
   CHECK_PARSE_BOOL(BinPackArguments);
   CHECK_PARSE_BOOL(BinPackParameters);