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

[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!