Page MenuHomePhabricator

clang-format: Add new style option AlignConsecutiveMacros
Needs ReviewPublic

Authored by VelocityRa on Jan 8 2017, 7:50 PM.
Tags
  • Restricted Project
Tokens
"Like" token, awarded by raziel."Like" token, awarded by MyDeveloperDay."Like" token, awarded by Bmooij."Love" token, awarded by veegee."Like" token, awarded by Maldus512."Love" token, awarded by micah-s."Like" token, awarded by AlexAltea."Love" token, awarded by ve4edj."Like" token, awarded by lassi.niemisto."Like" token, awarded by Dr-Emann."Love" token, awarded by markoshorro."Like" token, awarded by elvinio."Like" token, awarded by TimF."Like" token, awarded by tannewt."Like" token, awarded by jzr."Love" token, awarded by rian.sanderson.

Details

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

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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

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

enyquist edited the summary of this revision. (Show Details)Apr 11 2018, 1:30 PM

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 added a comment.Apr 11 2018, 11:19 PM

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 :-)

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

klimek added inline comments.Apr 20 2018, 1:05 AM
lib/Format/WhitespaceManager.cpp
438

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.

Dr-Emann added a subscriber: Dr-Emann.

I am waiting for this feature. Any update?

My team is waiting for this feature, too.

@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 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.

@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 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 ;)

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 #.

ve4edj added a subscriber: ve4edj.
mewmew added a subscriber: mewmew.Oct 6 2018, 8:31 AM

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.

micah-s added a subscriber: micah-s.Oct 7 2018, 6:38 AM
AlexAltea added a subscriber: AlexAltea.
veegee awarded a token.Nov 5 2018, 4:43 AM
veegee added a subscriber: veegee.

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 added a comment.EditedNov 26 2018, 10:46 AM

@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.

13rac1 added a subscriber: 13rac1.Dec 1 2018, 10:55 PM
Bmooij added a subscriber: Bmooij.Dec 28 2018, 8:36 AM

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 commandeered this revision.Jan 28 2019, 8:01 PM
VelocityRa added a reviewer: enyquist.
VelocityRa updated this revision to Diff 184010.EditedJan 28 2019, 8:08 PM

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.

VelocityRa marked 8 inline comments as done.Jan 29 2019, 7:06 AM

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 added a comment.EditedJan 29 2019, 8:11 PM

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 :)

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.

raziel added a subscriber: raziel.
mikel added a subscriber: mikel.Feb 13 2019, 9:04 PM

Is there anything holding this up that could use help?

@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?

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.

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.

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?

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.

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.

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.

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.

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.

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.

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

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!

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

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

437

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

442

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

473

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

497

llvm style: use upper-case I and E.

500

Why set EndOfSequence outside the if below?

512

What's the reason for this?

VelocityRa marked 11 inline comments as done.Thu, Mar 14, 9:17 AM

Waiting for further feedback before pushing an update.

lib/Format/WhitespaceManager.cpp
436

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

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

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

473

Correct. My bad.

500

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

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

klimek added inline comments.Wed, Mar 20, 3:51 AM
lib/Format/WhitespaceManager.cpp
436

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

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

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

500

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

VelocityRa updated this revision to Diff 191493.EditedWed, Mar 20, 7:44 AM
VelocityRa marked 13 inline comments as done.

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

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 added inline comments.Fri, Mar 22, 7:21 AM
lib/Format/WhitespaceManager.cpp
482

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.

Right, how's that?