This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Add Left/Right Const fixer capability
ClosedPublic

Authored by MyDeveloperDay on Nov 3 2019, 7:52 AM.
Tokens
"Like" token, awarded by Folling.

Details

Summary

Developers these days seem to argue over east vs west const like they used to argue over tabs vs whitespace or the various bracing style. These previous arguments were mainly eliminated with tools like clang-format that allowed those rules to become part of your style guide. Anyone who has been using clang-format in a large team over the last couple of years knows that we don't have those religious arguments any more, and code reviews are more productive.

https://www.youtube.com/watch?v=fv--IKZFVO8
https://mariusbancila.ro/blog/2018/11/23/join-the-east-const-revolution/
https://www.youtube.com/watch?v=z6s6bacI424

The purpose of this revision is to try to do the same for the East/West const discussion. Move the debate into the style guide and leave it there!

In addition to the new ConstStyle: Right or ConstStyle: Left there is an additional command-line argument --const-style=left/right which would allow an individual developer to switch the source back and forth to their own style for editing, and back to the committed style before commit. (you could imagine an IDE might offer such a switch)

The revision works by implementing a separate pass of the Annotated lines much like the SortIncludes and then create replacements for constant type declarations.

Diff Detail

Event Timeline

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

It's very difficult to use a compile_commands database if you can't
actually check out all the code and a remote service builds it for you.

FWIW, if you use the compile commands database, the only thing you need to do on the command line is specify the checks to enable or disable.

The project I work on doesn't have/use compile commands databases? if you are make based system or some other legacy build system you just may not have this, so I don't want to limit the uses to just those projects that do.

These are both fair criticisms (that I agree with!). However, adding an entirely new tool is a pretty heavy approach and I think we should only do it when there's a strong use case (and there may be such a strong case in this instance). However, I just now realize that we've added other tools for this sort of thing in the past -- we have clang-change-namespace and clang-reorder-fields which are not entirely unrelated to this same concept, so there's something like precedent here.

so there's something like precedent here

I think its worth mentioning, that my personal preference would STILL be to land this inside clang-format with default configuration of "OFF", where there is also significant existing precedent for passes that change non whitespace and even add tokens.

I still believe clang-format is the best location, I think its the most optimal, and I think its the fairest (because those that want it get it, and those that don't aren't forced to have it). But doing this as another tool would be a compromise, in my view an inferior one to it being in clang-format, but at least we could set out clear goals where allowing code modification was the intent (as this seems to be the major sticking point)

I would be interested to know how many people would be unhappy if we stated that "sorting includes" and "namespace comments" had to be removed from clang-format and into the new tool! I am thinking it would be fairly significant. (I'm not suggesting we would, just making a point)

So you know, I'm pushing because I'm being ask privately to land this, because I assume people want to use it, but maybe don't want to voice their opinions publicly.

I don't want to fragment the community by pushing an agenda (something I see you seem to care about), but I would also like to think that those of us who contribute to clang-format regularly can help shape a future for it moving forward.

so there's something like precedent here

I think its worth mentioning, that my personal preference would STILL be to land this inside clang-format with default configuration of "OFF", where there is also significant existing precedent for passes that change non whitespace and even add tokens.

Fair.

I still believe clang-format is the best location, I think its the most optimal, and I think its the fairest (because those that want it get it, and those that don't aren't forced to have it). But doing this as another tool would be a compromise, in my view an inferior one to it being in clang-format, but at least we could set out clear goals where allowing code modification was the intent (as this seems to be the major sticking point)

I would be interested to know how many people would be unhappy if we stated that "sorting includes" and "namespace comments" had to be removed from clang-format and into the new tool! I am thinking it would be fairly significant. (I'm not suggesting we would, just making a point)

I would be in favor of such a migration for sorting includes. I want my code formatting tool to not break my code. I recognize the tool already potentially breaks code when reordering fragile includes, but I think that was a mistake that should not be purposefully repeated without the community actively saying that's the direction they want the tool to go in. Personally, I'd be opposed to such a direction (each new breaking transformation supplied by the tool makes the tool that much less trustworthy in common use cases like CI pipelines), but perhaps the community has an appetite for such a change.

(Btw, I don't see how namespace comments really relate -- those don't break code that I'm aware of. If the concern is "but they're not whitespace"; they are as far as the compiler is concerned.)

So you know, I'm pushing because I'm being ask privately to land this, because I assume people want to use it, but maybe don't want to voice their opinions publicly.

I don't want to fragment the community by pushing an agenda (something I see you seem to care about), but I would also like to think that those of us who contribute to clang-format regularly can help shape a future for it moving forward.

I am not the code owner for this tool, so I think the decision ultimately lands on @rsmith as we have no code owner specifically for this component. However: https://reviews.llvm.org/D69764#2056105 so it's not like I'm alone in my concerns.

I already said I would like that in clang-format and would directly add that to my config.

I also think that there should be no problem in having that in clang-format, include sorting has a bigger chance of breaking code, yeah only with poorly designed headers but these exist, something which can not be fixed in clang-format itself, but only with // clang-format off or a elaborated sorting configuration. Breaking const moves can be fixed in clang-format and since it is off by default I really don't see a problem.

For the option: Maybe it should directly be prepared to sort all kinds of qualifiers, even if they are not added in this patch.

clang/docs/ClangFormatStyleOptions.rst
1378

I also think Left and Right are the better choices, but I would be full in favor of also parsing West and East. :)

clang/lib/Format/EastWestConstFixer.cpp
294

I would be in favor of just const int.

clang/lib/Format/Format.cpp
961

These are not needed if the LLVM style is already Leave.

I think its worth mentioning, that my personal preference would STILL be to land this inside clang-format with default configuration of "OFF",
I would be interested to know how many people would be unhappy if we stated that "sorting includes" and "namespace comments" had to be removed from clang-format and into the new tool!
So you know, I'm pushing because I'm being ask privately to land this, because I assume people want to use it, but maybe don't want to voice their opinions publicly.

I was a quiet observer up to this point, but I hope that my voice will help asses "what community wants", at least as an additional sample point. I also hope that the "community" is not only defined here as "open-source projects", but also all those poor souls working for closed-source projects ;)

I started observing this review when I was still working for another company and we were really interested in this feature. We were using clang-format extensively on several multi-milion LOC projects with total number of developers going into hundreds. "sorting includes" and "namespace comments" were one of the features that we enabled in our code bases as soon as they landed. We even quietly helped to improve include sorting here, in clang-format, squashing some bugs or extending the use-cases. We had those features enabled even with known limitations because clang-format in such large projects is not only a "simple whitespace formatting tool" - it becomes emotionless policing tool and time-saver during code reviews.

It had never crossed our minds that "sorting includes" or "namespace comments" should be separate tools. And if we did create separate tools as opposed to extending clang-format, I am 90% sure they would stay closed behind corporate doors. Another point of view is how easy it is to integrate clang-format as an existing well-known tool into various IDEs vs integrating a set of separate code-formatting programs that can have their own amusing side-effects when launched in different order.

land this inside clang-format with default configuration of "OFF",

yes please. IMHO merging this and letting it mature over time as people use it is the best way forward vs risking various forks to be maintained by teams internally and spooking potential contributors

I would be interested to know how many people would be unhappy

at least a few hundreds

because I assume people want to use it

a cure for that itch, yes

Because I don't want to move this review on other than to maintain it for future rebasing, I've created a new review for the concept of the new tool D105943: [clang-format++] Create a new variant of the clang-format tool to allow additional code mutating behaviour such as East/West Const Fixer

This is not what I wanted to do, but lets start here and see if people dislike the thought of a new tool more than they dislike the concept of putting it into clang-format in the first place.

Interesting reference point here https://blog.jetbrains.com/rscpp/2021/03/30/resharper-cpp-2021-1-syntax-style

Kind of suggests that Resharper brings many of these "mutating" capabilities into the same tool that fixes your style. I'd be really interest to know if they do that by having to have a compilation command database or if they make those substitutions based on on the tokens and not having full semantic information

I've been trying to make my opinion on this patch for the last few weeks...
I was pretty much opposed to introducing non-whitespace chances previously, but I'm reviewing my standpoint.
As mentioned already, there are precedents (include sorting, namespace comments, long string splitting).
I'm however even more wary of adding yet another tool that will be almost the same as clang-format. It could work if it were a drop-in replacement of clang-format, but that seems to be very much wishful thinking to me.
First, maintenance burden to verify that the two don't diverge somehow. Secondly, the drop-in replacement wouldn't be possible without changing clang-format itself (e.g. to ignore style options that are for "clang-format++" only). Also, it might divide the users into clang-format camp and clang-format++ camp (which may or may not be a problem).
Lastly, I do think that clang-format can be as reliable with this patch as it's now. Breaking code is of course possible but that's the case of many style options. And these are bugs that will eventually get fixed. It's of course important that this option doesn't break anything ever by default, but given that the default is Leave, and it's implemented as an additional pass, that should be the case.
Also, I'd be a bit surprised if people used it in CI immediately after this feature has landed without verifying that it doesn't break anything on their codebase.

On the other hand, clang-tidy has a corresponding check. I do feel though that's a sort of heavyweight tool and much less used than clang-format. Also, the placing of const qualifier is by many (at least in my circles) associated to the latter tool.

So yes, I'm in favour of landing this patch (though not exactly in the current form, I'd prefer more future-proof options for instance, not only handling const).
My (longish) 2 cents.

+1 for not only handling "const". I've often tried getting the various bits
that appertain to a declaration (static const volatile constexpr inline
consteval) sorted in a consistent order - that makes them much more
greppable.

Different patch, I expect, though.

MyDeveloperDay added a comment.EditedJul 14 2021, 6:46 AM

So yes, I'm in favour of landing this patch (though not exactly in the current form, I'd prefer more future-proof options for instance, not only handling const)

I am in agreement, but I don't want to put more effort into improving the current design of this patch to handle more use-cases and options UNTIL we round out on the go/no go decision.

From my perspective the use of violate is lower priority as its used like less than 0.01% as often as const. but I definitely think that we can add additional options on the lines of ReSharper to give even greater flexibility

I wanted to address your other points so we at least can be aligned as I respect your opinion.

I'm however even more wary of adding yet another tool that will be almost the same as clang-format.

Also agreed, but I see no other way forward.

It could work if it were a drop-in replacement of clang-format, but that seems to be very much wishful thinking to me.

This is exactly my suggestion. D105943: [clang-format++] Create a new variant of the clang-format tool to allow additional code mutating behaviour such as East/West Const Fixer will be a new clang-format for those that accept the fact it will modify, why those who can't just can't NOT turn the options on in the first place, I'm not quite sure, but I'm trying to be inclusive of everyone and find a road to a solution which is good for those that want it and those that don't

If we create a new tool, I recommend you, I and some of the other clang-format regulars also be the CODE_OWNERS so we can innovate without feeling stifled.

First, maintenance burden to verify that the two don't diverge somehow.

My aim would be to move the grunt of the ClangFormat.cpp into lib/Format, so both processes could share them, the main() would effective become calling the same submain() but with a "Yes please mutate all you like variable"

Secondly, the drop-in replacement wouldn't be possible without changing clang-format itself (e.g. to ignore style options that are for "clang-format++" only).

No I wouldn't do this, the old clang-format would ignore mutating passes the new one wouldn't, the style options would remain for both so they can both share a common .clang-format file (lets remember I don't want to do it this way!)

Also, it might divide the users into clang-format camp and clang-format++ camp (which may or may not be a problem).

Of course, and it would "Fragment the Community", but this is what happens when some want to innovate and others like it the way it is (we see this all the time from mailing lists to bug tracker), but it doesn't have to be this way, we could simply have one binary to rule them all and keep in the community that you and I spend a significant amount of our time giving back to.

Lastly, I do think that clang-format can be as reliable with this patch as it's now.

Me too.

Also, I'd be a bit surprised if people used it in CI immediately after this feature has landed without verifying that it doesn't break anything on their codebase.

No I use CI in an advisory capacity with the -n or (--dry-run) option just to highlight violations not change them)

It would probably be better to make the config option names for passes that
may mutate whitespace be prefixed with MaybeIncorrect than fork the tool.
Scary options are better than forks.

So yes, I'm in favour of landing this patch (though not exactly in the current form, I'd prefer more future-proof options for instance, not only handling const)

I am in agreement, but I don't want to put more effort into improving the current design of this patch to handle more use-cases and options UNTIL we round out on the go/no go decision.

From my perspective the use of violate is lower priority as its used like less than 0.01% as often as const. but I definitely think that we can add additional options on the lines of ReSharper to give even greater flexibility

I would basically enumerate the qualifiers we can have and let the user decide the desired order.

If we create a new tool, I recommend you, I and some of the other clang-format regulars also be the CODE_OWNERS so we can innovate without feeling stifled.

On another note, I think you could already now run for that position of clang-format. :)

It would probably be better to make the config option names for passes that
may mutate whitespace be prefixed with MaybeIncorrect than fork the tool.
Scary options are better than forks.

I don't feel this is necessary.

steveire added a comment.EditedAug 7 2021, 2:21 PM

Making a separate tool for this makes no sense. Especially as you are only proposing it to satisfy one (or are there more?) vocal objector.

The objections to this make no sense. If you don't want to use it, then don't enable it. That principle applies whether "the way to enable it" is "enable this option" or "use this other tool instead". The latter is just more inconvenient. There is no other difference. Don't impose that on users just because of unreasonable objection.

Maintainership sometimes means dismissing objections that make no sense.

It seems to me that there are two ways forward:

  1. Land this (Yes. Do this. This is what makes sense)
  • Expose this feature in another tool with a very similar name
  • Somehow tell users that the other tool exists and should be used instead
  • Realize some years from now that we have two tools where we should have one and this is inconvenient for users
  • Merge your new tool into clang-format
  • Rejoice and wish this had been done in 2020

The outcome is the same, but it's a real disrespect to users to not just land this in clang-format now. The objection to doing so makes no sense, so it should be considered and dismissed (it has not been ignored).

@steveire, thanks for your comments, I also agree that a second tool shouldn't be needed, especially as this functionality is off by default (and needs to stay that way). This was my suggestion for a compromise.

Its a tough call, but ultimately no on ever gave this a LGTM so I couldn't proceed anyway. Whilst I think we can cover the other qualifiers with a similar approach I don't want to waste more of my time until the fundamental question has been answered if its ok for clang-format to insert/modify the order of tokens and that is more a collective decision.

It was suggested to me that I write up and RFC on the matter, I'm not a massive fan of those as I don't really see a formal process for handling them (the conversation seems to turn into a series of personal preferences then dwindles and dies without conclusion). But if people think it would solve something I guess I could try.

From my perspective I'm more interested in the views of the major clang-format contributors (@curdeius, @krasimir, @owenpan , @sammccall, @HazardyKnusperkeks and people like yourselves who have put time and effort into blogging about these tools), ultimately it will be us who have to handle the impact of this (and maybe the #clangd people) and I don't want to inconvenience them or make work for them.

So here is what is needed to land this in my view:

  1. I'd want at least 2 to 3 of the current reviewers to LGTM.
  2. I'd want 1 person (like yourself) a community contributor to also give their approval (I guess I have that conceptually from you!)

or

  1. @klimek or @djasper to give us an its ok for "clang-format to insert/modify the order of tokens" in a controlled manner decision
  2. 1 LGTM

Whilst I respect the views of those few who have objected, they don't to my knowledge make significant contributions to clang-format (not in the last 5-6 years I've been following it anyway)

I feel this team owns the decision as we are the ones that look after it, warts and all. We'll always have a situation where some don't agree, thats ok, but I feel there is more agreement that this funcationality is ok rather than not.

Until one of those 2 things happens, we are in limbo.

klimek added a comment.Aug 9 2021, 4:00 AM

@steveire, thanks for your comments, I also agree that a second tool shouldn't be needed, especially as this functionality is off by default (and needs to stay that way). This was my suggestion for a compromise.

Its a tough call, but ultimately no on ever gave this a LGTM so I couldn't proceed anyway. Whilst I think we can cover the other qualifiers with a similar approach I don't want to waste more of my time until the fundamental question has been answered if its ok for clang-format to insert/modify the order of tokens and that is more a collective decision.

It was suggested to me that I write up and RFC on the matter, I'm not a massive fan of those as I don't really see a formal process for handling them (the conversation seems to turn into a series of personal preferences then dwindles and dies without conclusion). But if people think it would solve something I guess I could try.

From my perspective I'm more interested in the views of the major clang-format contributors (@curdeius, @krasimir, @owenpan , @sammccall, @HazardyKnusperkeks and people like yourselves who have put time and effort into blogging about these tools), ultimately it will be us who have to handle the impact of this (and maybe the #clangd people) and I don't want to inconvenience them or make work for them.

So here is what is needed to land this in my view:

  1. I'd want at least 2 to 3 of the current reviewers to LGTM.
  2. I'd want 1 person (like yourself) a community contributor to also give their approval (I guess I have that conceptually from you!)

or

  1. @klimek or @djasper to give us an its ok for "clang-format to insert/modify the order of tokens" in a controlled manner decision
  2. 1 LGTM

Whilst I respect the views of those few who have objected, they don't to my knowledge make significant contributions to clang-format (not in the last 5-6 years I've been following it anyway)

I feel this team owns the decision as we are the ones that look after it, warts and all. We'll always have a situation where some don't agree, thats ok, but I feel there is more agreement that this funcationality is ok rather than not.

Until one of those 2 things happens, we are in limbo.

I think we had a really good, inclusive discussion on this change, so I don't think an RFC would add anything to it.

I think we have consensus on, if we want this, we:

  1. want it behind an option that is not enabled in default-configs
  2. we want users to understand that this has the potential to introduce bugs

I also think this is a feature that seems useful enough for a large enough group, to provide it in clang-format.
I'd put a lot of weight on @MyDeveloperDay's evaluation, as in the end, he's currently taking the most active role in keeping clang-format alive & well.

One idea for clang-format going forward is: can we make it easier for people to understand what flags can introduce non-whitespace-changes? E.g. have flag name prefixes like semantic-*.

So, I'm generally OK with this.

One minor nit I have is that I'd personally add unit tests to the EastWestConstFixer, as it's quite a large class, but given that I'll duck out after this comment, I'll not insist :)

It was suggested to me that I write up and RFC on the matter, I'm not a massive fan of those as I don't really see a formal process for handling them (the conversation seems to turn into a series of personal preferences then dwindles and dies without conclusion). But if people think it would solve something I guess I could try.

From my perspective I'm more interested in the views of the major clang-format contributors (@curdeius, @krasimir, @owenpan , @sammccall, @HazardyKnusperkeks and people like yourselves who have put time and effort into blogging about these tools), ultimately it will be us who have to handle the impact of this (and maybe the #clangd people) and I don't want to inconvenience them or make work for them.

So here is what is needed to land this in my view:

  1. I'd want at least 2 to 3 of the current reviewers to LGTM.
  2. I'd want 1 person (like yourself) a community contributor to also give their approval (I guess I have that conceptually from you!)

or

  1. @klimek or @djasper to give us an its ok for "clang-format to insert/modify the order of tokens" in a controlled manner decision
  2. 1 LGTM

Whilst I respect the views of those few who have objected, they don't to my knowledge make significant contributions to clang-format (not in the last 5-6 years I've been following it anyway)

I feel this team owns the decision as we are the ones that look after it, warts and all. We'll always have a situation where some don't agree, thats ok, but I feel there is more agreement that this funcationality is ok rather than not.

Until one of those 2 things happens, we are in limbo.

I think we had a really good, inclusive discussion on this change, so I don't think an RFC would add anything to it.

FWIW, the RFC I requested off-list wasn't about east/west const fixing, it was about whether the community was okay with a new direction of clang-format that breaks code or not. This is a shift in direction from the original design of clang-format. The waters were muddied somewhat by include sorting, but in my totally unscientific polls on the matter, I found basically no one concerned about include sorting while I found a not-insignificant number of people who were concerned about qualifier formatting and breakage in general. The prevailing sentiments were that breakage from include sorting demonstrates fragility with your code, but qualifier shuffling may or may not. That said, I did find that about 1/3 of the people were fine with breakage in general so long as it's mandated to be an opt-in feature as a matter of policy (and about 65% were uncomfortable with breakage from a code formatting tool in general). Take that "data" with a heaping grain of salt, it was a Twitter poll with ~150 respondents.

Personally, I still think an RFC (as frustrating of a process as it can be) is valuable because this strikes me as a policy change for a very popular tool. I highly doubt much of the community is following this review with that in mind, let alone the greater user base for clang-format.

I think we have consensus on, if we want this, we:

  1. want it behind an option that is not enabled in default-configs
  2. we want users to understand that this has the potential to introduce bugs

+1

I also think this is a feature that seems useful enough for a large enough group, to provide it in clang-format.
I'd put a lot of weight on @MyDeveloperDay's evaluation, as in the end, he's currently taking the most active role in keeping clang-format alive & well.

One idea for clang-format going forward is: can we make it easier for people to understand what flags can introduce non-whitespace-changes? E.g. have flag name prefixes like semantic-*.

There are orthogonal ideas here. 1) I think having the flag name clearly show that this may break the user's code is a good idea. 2) I do not think this review should set a new policy that breaking changes in clang-format are acceptable unless there is a larger community discussion specifically about that point.

So, I'm generally OK with this.

I continue to oppose the decision on the grounds that code formatting tools should not be opinionated on the formatting of their input source code. This reduces the utility of a tool that's used by a *much* wider audience than people represented on this review. That said, I'll abide by the consensus position.

For my part, I'm convinced and now +1 (or at least +0.5) on this being OK to include.
In users' minds this is a formatting/style operation, and the UX of clang-format and its integrations is much better than clang-tidy.

Implementation quality problems are a risk, but can be mitigated:

  • today: clang-format itself is mature and enforced on some projects. And this feature isn't as mature yet, and can be dangerous. Guarding the feature as experimental by a flag and/or prefixed config names seems like a good way to tell people that.
  • forever: if this can never be made sufficiently robust, it should not be promoted to a "standard" feature. This would be sad/awkward, but the history of clang-format suggests it's not that likely.

@MyDeveloperDay: sorry that it's been (and continues to be) hard to get consensus here. It's not surprising: there are strong arguments in both directions, and they appeal to different values.
@steveire: I don't think it's true or helpful to suggest the downsides aren't real, or to isolate people as holdouts.

I think we had a really good, inclusive discussion on this change, so I don't think an RFC would add anything to it.

I agree with this. I'd be surprised if the balance of arguments about const-fixing was substantially different.
A more abstract RFC about direction seems unikely to be productive. I think *all* uses of clang-format can break certain code, it's a (large) difference in degree. So the concrete details matter.

erichkeane requested changes to this revision.Aug 9 2021, 6:12 AM
erichkeane added a subscriber: erichkeane.

I've just been watching this from the sideline, but the cases where this breaks code are unacceptable for this tool, it is a complete direction change for the tool, and making that direction change silently on a review of a 15 month patch, where TWO code owners have said 'no' for that reason is absurd.

I use this tool daily as a part of my 'upload' script, having it silently bust code between when I validate it and when I upload it is terrible, and makes the tool unusable for my purposes. If we change this direction without a full RFC, my next step is going to be an RFC to remove clang-format from the check-in requirements of the entire LLVM project.

This revision now requires changes to proceed.Aug 9 2021, 6:12 AM

I've just been watching this from the sideline, but the cases where this breaks code are unacceptable for this tool, it is a complete direction change for the tool, and making that direction change silently on a review of a 15 month patch, where TWO code owners have said 'no' for that reason is absurd.

I use this tool daily as a part of my 'upload' script, having it silently bust code between when I validate it and when I upload it is terrible, and makes the tool unusable for my purposes. If we change this direction without a full RFC, my next step is going to be an RFC to remove clang-format from the check-in requirements of the entire LLVM project.

This and other potentially other mutating options would and MUST in my view ALWAYS be 100% "off by default" for all default style options (as -fix is for clang-tidy), it would be a purely "opt in" basis. (via .clang-format or command line)

I personally use this in a non modifying way "using the -dry-run mode" to catch new"east/const violations" and report failure back rather than "change the code itself"

I would not expect clang-format usage to change unless someone specially opted in to using it.

I've just been watching this from the sideline, but the cases where this breaks code are unacceptable for this tool, it is a complete direction change for the tool, and making that direction change silently on a review of a 15 month patch, where TWO code owners have said 'no' for that reason is absurd.

I use this tool daily as a part of my 'upload' script, having it silently bust code between when I validate it and when I upload it is terrible, and makes the tool unusable for my purposes. If we change this direction without a full RFC, my next step is going to be an RFC to remove clang-format from the check-in requirements of the entire LLVM project.

This and other potentially other mutating options would and MUST in my view ALWAYS be 100% "off by default" for all default style options (as -fix is for clang-tidy), it would be a purely "opt in" basis. (via .clang-format or command line)

I personally use this in a non modifying way "using the -dry-run mode" to catch new"east/const violations" and report failure back rather than "change the code itself"

I would not expect clang-format usage to change unless someone specially opted in to using it.

That seems just as bad, if not worse. Clang-format isn't an analysis tool, its a format tool. If you have an option that can only reasonably be run in 'dry-run' mode, it seems that putting it in a 'format' tool is the wrong place.

I've just been watching this from the sideline, but the cases where this breaks code are unacceptable for this tool, it is a complete direction change for the tool, and making that direction change silently on a review of a 15 month patch, where TWO code owners have said 'no' for that reason is absurd.

I use this tool daily as a part of my 'upload' script, having it silently bust code between when I validate it and when I upload it is terrible, and makes the tool unusable for my purposes. If we change this direction without a full RFC, my next step is going to be an RFC to remove clang-format from the check-in requirements of the entire LLVM project.

Can I just say, marking this review as requiring changes, I presume because you don't agree with it conceptually isn't very helpful to the consensus building process, unless you have an inline comment of something you think is wrong. What changes are you requesting?

I've tried to find compromises to mitigate peoples strong views, I know this is contentious, I've not tried to rush it in. I am a major contributor to clang-format, and I'd like to continue to move it forward..

as @klimek mentioned I'm one of the people really trying to help look after clang-format, I wouldn't do anything that I think damages it or its reputation, but I think there is value in this and other proposed changes. I know some people disagree but we also have to recognise that some agree too!

A "no" is a "no for everyone", a "yes" is a "yes and no" based on the configuration, I know what I think is the fairer approach.

I've just been watching this from the sideline, but the cases where this breaks code are unacceptable for this tool, it is a complete direction change for the tool, and making that direction change silently on a review of a 15 month patch, where TWO code owners have said 'no' for that reason is absurd.

I use this tool daily as a part of my 'upload' script, having it silently bust code between when I validate it and when I upload it is terrible, and makes the tool unusable for my purposes. If we change this direction without a full RFC, my next step is going to be an RFC to remove clang-format from the check-in requirements of the entire LLVM project.

This and other potentially other mutating options would and MUST in my view ALWAYS be 100% "off by default" for all default style options (as -fix is for clang-tidy), it would be a purely "opt in" basis. (via .clang-format or command line)

I personally use this in a non modifying way "using the -dry-run mode" to catch new"east/const violations" and report failure back rather than "change the code itself"

I would not expect clang-format usage to change unless someone specially opted in to using it.

That seems just as bad, if not worse. Clang-format isn't an analysis tool, its a format tool. If you have an option that can only reasonably be run in 'dry-run' mode, it seems that putting it in a 'format' tool is the wrong place.

This is exactly what the "llvm-premerge checks" do!, why can't it also be an analysis tool? your own usage scenario may not be the same as everyone elses, that doesn't make it wrong!

If you have an option that can only reasonably be run in 'dry-run' mode

This isn't true, I successfully "east consted" all of clang causing no build errors and from what I can tell no unit test failure either, I'm just saying using it in dry-run mode is an equally useful usage scenario.

I've just been watching this from the sideline, but the cases where this breaks code are unacceptable for this tool, it is a complete direction change for the tool, and making that direction change silently on a review of a 15 month patch, where TWO code owners have said 'no' for that reason is absurd.

I use this tool daily as a part of my 'upload' script, having it silently bust code between when I validate it and when I upload it is terrible, and makes the tool unusable for my purposes. If we change this direction without a full RFC, my next step is going to be an RFC to remove clang-format from the check-in requirements of the entire LLVM project.

Can I just say, marking this review as requiring changes, I presume because you don't agree with it conceptually isn't very helpful to the consensus building process, unless you have an inline comment of something you think is wrong. What changes are you requesting?

I've tried to find compromises to mitigate peoples strong views, I know this is contentious, I've not tried to rush it in. I am a major contributor to clang-format, and I'd like to continue to move it forward..

as @klimek mentioned I'm one of the people really trying to help look after clang-format, I wouldn't do anything that I think damages it or its reputation, but I think there is value in this and other proposed changes. I know some people disagree but we also have to recognise that some agree too!

A "no" is a "no for everyone", a "yes" is a "yes and no" based on the configuration, I know what I think is the fairer approach.

My 'requires changes' is that this needs an LLVM-project-level RFC to change the charter of one of its projects, doing so in a 15 month long patch, against the wishes of TWO maintainers is a violation of the LLVM community practice. I'm completely willing to disagree-and-commit here once that happens, but allowing this patch in without that decision being made intentionally by the project seems like a violation of trust.

My 'requires changes' is that this needs an LLVM-project-level RFC to change the charter of one of its projects, doing so in a 15 month long patch, against the wishes of TWO maintainers is a violation of the LLVM community practice. I'm completely willing to disagree-and-commit here once that happens, but allowing this patch in without that decision being made intentionally by the project seems like a violation of trust.

Ok, thats fair and thank you for verbalising what the changes are. I'm not closed to the idea of the RFC just didn't want to go down that road if it just ended up with 2 opposing views and not getting to a conclusion.

I will challenge a couple of things:

  1. I'm not sure there is currently a "charter" that says we won't modify the contents of a TU, and actually in my view that has already changed when we added. (include sorting, namespace comments, javascript requoter, trailing comment inserter).
  1. I agree if we want to use this as formalising that "change" in charter then I'm ok to try via the RFC but I think we'll get 2 very opposing views, and likely no concencus. So I don't want to just cause a rift in the community any more than this is already.
  1. As for the "TWO maintainers", I don't deny their extremely excellent contributions, far greater than mine could ever be. But in fairness they are not frequent maintainers here! On the other hand I am and have been for a number of years. When @djasper and @klimek stepped back a bit, I've really tried to help by filling even a little, the void of their enormous shoes.

I can't even think of emulating all those peoples amazing efforts, but I do this in my free time as I assume other do, and I like to think there is value in me continuing to improve and debug clang-format. So I'd like to think my view isn't disregarded just because others with more muscle disagree, I mean I assumed this was at least a democracy where we could find a fair concencus.

My 'requires changes' is that this needs an LLVM-project-level RFC to change the charter of one of its projects, doing so in a 15 month long patch, against the wishes of TWO maintainers is a violation of the LLVM community practice. I'm completely willing to disagree-and-commit here once that happens, but allowing this patch in without that decision being made intentionally by the project seems like a violation of trust.

Ok, thats fair and thank you for verbalising what the changes are. I'm not closed to the idea of the RFC just didn't want to go down that road if it just ended up with 2 opposing views and not getting to a conclusion.

I will challenge a couple of things:

  1. I'm not sure there is currently a "charter" that says we won't modify the contents of a TU, and actually in my view that has already changed when we added. (include sorting, namespace comments, javascript requoter, trailing comment inserter).
  1. I agree if we want to use this as formalising that "change" in charter then I'm ok to try via the RFC but I think we'll get 2 very opposing views, and likely no concencus. So I don't want to just cause a rift in the community any more than this is already.

For better or worse, RFCs are our way of changing these things. RFCs definitely succeed after discussion sometimes. _I_ would be completely against this patch unless some level of community consensus was formed via RFC, and I believe a few others above have made the same point.

  1. As for the "TWO maintainers", I don't deny their extremely excellent contributions, far greater than mine could ever be. But in fairness they are not frequent maintainers here! On the other hand I am and have been for a number of years. When @djasper and @klimek stepped back a bit, I've really tried to help by filling even a little, the void of their enormous shoes.

I can't even think of emulating all those peoples amazing efforts, but I do this in my free time as I assume other do, and I like to think there is value in me continuing to improve and debug clang-format.

I was referring to @rsmith and @aaron.ballman (to clarify), both are maintainers in 'clang', the former of which is the 'superset' maintainer of this format project based on its directory. While Aaron is a peer-maintainer to this project, his contributions are immense, and his points are too-well-reasoned and motivated to be dismissed.

So I'd like to think my view isn't disregarded just because others with more muscle disagree, I mean I assumed this was at least a democracy where we could find a fair concencus.

It _IS_ a democracy where we can find a fair consensus! And the mechanism with which to obtain said fair consensus is an RFC.

It _IS_ a democracy where we can find a fair consensus! And the mechanism with which to obtain said fair consensus is an RFC.

Then I think in the interest of finding one we should start with the RFC.

It _IS_ a democracy where we can find a fair consensus! And the mechanism with which to obtain said fair consensus is an RFC.

Then I think in the interest of finding one we should start with the RFC.

FWIW, if we gain consensus, I go from a -1000 on this to a +.75. From a technical perspective, I find it weird that we aren't handling ALL of the CV qualifiers.

From a "we should be inclusive" perspective, my understanding is that east==right, west==left is considered by some to be euro-centric due to its north-up bias. I don't feel strongly about it, but it at least gives me a somewhat significant preference for left/right vs east/west.

I was referring to @rsmith and @aaron.ballman (to clarify), both are maintainers in 'clang', the former of which is the 'superset' maintainer of this format project based on its directory. While Aaron is a peer-maintainer to this project, his contributions are immense, and his points are too-well-reasoned and motivated to be dismissed.

Just so we are clear I'm not dismissing anyone opinions, @arron.ballman and I have been going at this quite a bit both on and off list. I have huge respect for these people, (even if the defence of my review it might not seem so). I can't even think to emulate their contributions.

Ideally I'd like their blessing, but alas I fear that might be beyond my capabilities, but if there are things like the RFC that could even go some way to potentially them seeing a way forward that is mutually beneficial so that the door is even open a jar for this then I'm happy to try.

If ultimately the answer is "no" then I need to understand what can be done if anything, if that means a new tool then I'm ok with that as the compromise. Out right dismissal, I would be very sorry to see.

I find it weird that we aren't handling ALL of the CV qualifiers.

I will probably try and address this, I do have some ideas, but this will I believe complicate the implementation. For now I really want to understand if conceptually such a feature can be landed, not so much land it in entirely this form. if I can get some agreement and that really means that people need to generally be in agreement.

From a "we should be inclusive" perspective, my understanding is that east==right, west==left is considered by some to be euro-centric due to its north-up bias. I don't feel strongly about it, but it at least gives me a somewhat significant preference for left/right vs east/west.

I think this is partially historical because I was following the whole east const vs west const (https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rl-const,https://mariusbancila.ro/blog/2018/11/23/join-the-east-const-revolution/, https://www.youtube.com/watch?v=fv--IKZFVO8). I don't have a problem supporting one/both or either, I tend to think that is more arguing semantics.

After seeing those videos I wanted clang-format to end the east vs west war, like clang-format ended the whitespace war!,.... I didn't realise I'd cause a clang war in the process! oops!! ;-)

I find it weird that we aren't handling ALL of the CV qualifiers.

I will probably try and address this, I do have some ideas, but this will I believe complicate the implementation. For now I really want to understand if conceptually such a feature can be landed, not so much land it in entirely this form. if I can get some agreement and that really means that people need to generally be in agreement.

Understood.

From a "we should be inclusive" perspective, my understanding is that east==right, west==left is considered by some to be euro-centric due to its north-up bias. I don't feel strongly about it, but it at least gives me a somewhat significant preference for left/right vs east/west.

I think this is partially historical because I was following the whole east const vs west const (https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rl-const,https://mariusbancila.ro/blog/2018/11/23/join-the-east-const-revolution/, https://www.youtube.com/watch?v=fv--IKZFVO8). I don't have a problem supporting one/both or either, I tend to think that is more arguing semantics.

After seeing those videos I wanted clang-format to end the east vs west war, like clang-format ended the whitespace war!,.... I didn't realise I'd cause a clang war in the process! oops!! ;-)

I understand the origin of east/west in this case, I was around when those bracelets came out :) Someone internally pointed out the anti-inclusivity of the terminology, so I figured I'd bring it up.

I was referring to @rsmith and @aaron.ballman (to clarify), both are maintainers in 'clang', the former of which is the 'superset' maintainer of this format project based on its directory. While Aaron is a peer-maintainer to this project, his contributions are immense, and his points are too-well-reasoned and motivated to be dismissed.

Just so we are clear I'm not dismissing anyone opinions, @arron.ballman and I have been going at this quite a bit both on and off list. I have huge respect for these people, (even if the defence of my review it might not seem so). I can't even think to emulate their contributions.

Ideally I'd like their blessing, but alas I fear that might be beyond my capabilities, but if there are things like the RFC that could even go some way to potentially them seeing a way forward that is mutually beneficial so that the door is even open a jar for this then I'm happy to try.

If ultimately the answer is "no" then I need to understand what can be done if anything, if that means a new tool then I'm ok with that as the compromise. Out right dismissal, I would be very sorry to see.

Here are my thoughts put in one place.

0) I like the idea of being able to format qualifier location, and I think clang-format is the tool I would reach for to perform that work
.33) I wish this was generalized to handle all qualifiers rather than just const. (This is not a blocking wish.)
.66) I wish this used "left" and "right" rather than "east" and "west" for user-facing options and documentation. (This is Very Strong wish but I won't block the patch over it.)

  1. In general, I do not like the idea of my code formatting tool having opinions about the input source's existing formatting (I think valid code should remain valid). This is the area of concern causing me to ask for an RFC because I'm operating under the impression that not breaking code is (generally) the status quo for clang-format.
  2. If the community consensus is that formatting can break code (blanket permission, case by case basis, whatever), I'll definitely abide by that decision. I just want it to be an explicit decision from a wider audience than people who might be following this very long-running patch.
  3. The proposed opt-in nature of the check is a blessing and a curse. It alleviates some of the danger (it's not dangerous by default, you have to manually say you want it). But it doesn't eliminate the danger (code can still be broken) and it does raise the question of how often people opt into this sort of thing and whether it's worth the maintenance costs. I don't think any of us can answer those "do people opt in" questions though, so that's not a concern I think we should hold anything up over unless someone has actual data on usage of opt-in features for clang-format. I think opt-in alleviates enough of the danger that it's a worthwhile approach for this patch (and likely may be a good idea for a policy on future changes of the same kind).

tl;dr: if there's an RFC and the community says "breaking changes aren't that big of a deal to us", it tips me from "against" to "in favor" for this functionality.

I was referring to @rsmith and @aaron.ballman (to clarify), both are maintainers in 'clang', the former of which is the 'superset' maintainer of this format project based on its directory. While Aaron is a peer-maintainer to this project, his contributions are immense, and his points are too-well-reasoned and motivated to be dismissed.

Just so we are clear I'm not dismissing anyone opinions, @arron.ballman and I have been going at this quite a bit both on and off list. I have huge respect for these people, (even if the defence of my review it might not seem so). I can't even think to emulate their contributions.

Ideally I'd like their blessing, but alas I fear that might be beyond my capabilities, but if there are things like the RFC that could even go some way to potentially them seeing a way forward that is mutually beneficial so that the door is even open a jar for this then I'm happy to try.

If ultimately the answer is "no" then I need to understand what can be done if anything, if that means a new tool then I'm ok with that as the compromise. Out right dismissal, I would be very sorry to see.

Here are my thoughts put in one place.

0) I like the idea of being able to format qualifier location, and I think clang-format is the tool I would reach for to perform that work
.33) I wish this was generalized to handle all qualifiers rather than just const. (This is not a blocking wish.)
.66) I wish this used "left" and "right" rather than "east" and "west" for user-facing options and documentation. (This is Very Strong wish but I won't block the patch over it.)

  1. In general, I do not like the idea of my code formatting tool having opinions about the input source's existing formatting (I think valid code should remain valid). This is the area of concern causing me to ask for an RFC because I'm operating under the impression that not breaking code is (generally) the status quo for clang-format.
  2. If the community consensus is that formatting can break code (blanket permission, case by case basis, whatever), I'll definitely abide by that decision. I just want it to be an explicit decision from a wider audience than people who might be following this very long-running patch.
  3. The proposed opt-in nature of the check is a blessing and a curse. It alleviates some of the danger (it's not dangerous by default, you have to manually say you want it). But it doesn't eliminate the danger (code can still be broken) and it does raise the question of how often people opt into this sort of thing and whether it's worth the maintenance costs. I don't think any of us can answer those "do people opt in" questions though, so that's not a concern I think we should hold anything up over unless someone has actual data on usage of opt-in features for clang-format. I think opt-in alleviates enough of the danger that it's a worthwhile approach for this patch (and likely may be a good idea for a policy on future changes of the same kind).

tl;dr: if there's an RFC and the community says "breaking changes aren't that big of a deal to us", it tips me from "against" to "in favor" for this functionality.

Happy to go the RFC route if @MyDeveloperDay wants to do that.

FWIW, I don't understand the idea of the community being a democracy. It is not, and never was. Chris has designed an escalation process, when this was raised to him - I don't know whether this was ever made official or tested.
I also don't see this as a big change from clang-format's principles - C++ is (unfortunately) way too complex to not be able to introduce subtle bugs - the history of clang-format is full of them.

Note that I don't think the change will get consensus on an RFC, but not making the change will not get consensus, either - in that case, other than escalating to a clear higher decision power, or letting main contributors make the call, I don't know what to do.

Happy to go the RFC route if @MyDeveloperDay wants to do that.

I'm happy to do that (in fact I've written a draft), do people want to code review the RFC draft (as I could easily be presenting a biased viewpoint and don't want to) or are you happy to just comment on the RFC when it appears

I took the viewpoint of trying to discuss the concept of clang-format making code altering changes rather than focusing on east/west conversion.

Someone internally pointed out the anti-inclusivity of the terminology, so I figured I'd bring it up.

I apologise if I'm proliferating that, but could you educate me why its "anti-inclusive" I certainly didn't mean any offence in its use.

I'm absolutely OK about dropping east/west especially if we think its going to cause offence and we just want to avoid that.

Someone internally pointed out the anti-inclusivity of the terminology, so I figured I'd bring it up.

I apologise if I'm proliferating that, but could you educate me why its "anti-inclusive" I certainly didn't mean any offence in its use.

I'm absolutely OK about dropping east/west especially if we think its going to cause offence and we just want to avoid that.

I apologize in advance to anyone reading this who is affected by this, or knows more about this than I, but my understanding is that "west==left/east==right" propagates the legitimacy of a 'north up' map that was created as a response to the at-the-time-south-up maps having Africa/Asia "above" Europe. The result is (I think?) a propagation of a euro-centric/euro-supremacy ideology.

Again, thats my 'ELI5' understanding of it.

Someone internally pointed out the anti-inclusivity of the terminology, so I figured I'd bring it up.

I apologise if I'm proliferating that, but could you educate me why its "anti-inclusive" I certainly didn't mean any offence in its use.

I'm absolutely OK about dropping east/west especially if we think its going to cause offence and we just want to avoid that.

I apologize in advance to anyone reading this who is affected by this, or knows more about this than I, but my understanding is that "west==left/east==right" propagates the legitimacy of a 'north up' map that was created as a response to the at-the-time-south-up maps having Africa/Asia "above" Europe. The result is (I think?) a propagation of a euro-centric/euro-supremacy ideology.

Again, thats my 'ELI5' understanding of it.

Thank you I didn't know anything about that, so lets agree I'll make the changes to remove East/West to avoid any such issues. I might even gain a few fractions of a point from @aaron.ballman

erichkeane resigned from this revision.Aug 9 2021, 12:38 PM

Not sure if this will clear the 'needs revision', but an RFC has been sent.

This revision now requires review to proceed.Aug 9 2021, 12:38 PM
MyDeveloperDay retitled this revision from [clang-format] Add East/West Const fixer capability to [clang-format] Add Left/Right Const fixer capability.

Remove East/West terminology for inclusivity

MyDeveloperDay updated this revision to Diff 365456.EditedAug 10 2021, 6:16 AM
MyDeveloperDay marked 2 inline comments as done.
  • Add support for both const and volatile alignment
  • Change ConstPlacement to CVQualifierAlignment
  • Add CVQualifierOrder to allow control over const volatile and volatile const

by my reckoning I'm somewhat nearer to the +.99 from @aaron.ballman!

First off, I think it should be configured in a different way, to prepare the path for also formatting static, inline, etc.

If this is kept there should be tests on what happens if there is const or volatile more than once in the string list, and when there are typos. Should there be a command line warning on a typo or an unsupported identifier is in the list?

clang/lib/Format/Format.cpp
2450–2452
2456

I don't know if we should remove the braces here too.

But the loop should use references or StringRef, not copied strings.

  • elide the braces
  • use references

First off, I think it should be configured in a different way, to prepare the path for also formatting static, inline, etc.

To be honest I think if we wanted to do that we could do so by extending the "CVQualifierOrder" to support more keywords (To be honest I think the original requirement is mainly for const, I concede on the volatile as for the others I think I'd want to see how we get on in order to reduce the complexity. but it would be fun to try I guess!)

If this is kept there should be tests on what happens if there is const or volatile more than once in the string list, and when there are typos. Should there be a command line warning on a typo or an unsupported identifier is in the list?

Typos would be ignored by the "if", duplicates well that would impact because the keywords push though themselves hence the reversing based on direction.

I guess we COULD validate the options, but I think that would make the .clang-format file not very future proof if I check that it only contains "const volatile", let me check if other options every validate themselves in quite the same way.

(but I take your point), as always , thank you for the quick review @HazardyKnusperkeks

First off, I think it should be configured in a different way, to prepare the path for also formatting static, inline, etc.

I'm wondering if I'm gravitating more and more to what @aaron.ballman said way back, maybe I don't even need Left and Right but instead just

CVQualifierOrder: [ "static", "inline", "constexpr", "<type>" ,"const", "volatile"]

I wonder if I could simply split out the individual keywords as being "Left" or "Right" of the "<type>" and apply different alignments to different keywords at the same time....

  • Add CVQualifierOrder configuration validation and unit tests
MyDeveloperDay marked 3 inline comments as done.Aug 10 2021, 9:09 AM
MyDeveloperDay updated this revision to Diff 365519.EditedAug 10 2021, 9:47 AM
  • Rename the ConstFixer to QualifierAligmentFixer (as now we handle more than just const) and in preparation for handling others
  • Remove static class name from getTokenFromCVQualifier
  • Fix header guards
MyDeveloperDay added a comment.EditedAug 11 2021, 1:51 AM

With the introduction of CVQualifierOrder the need for CVQualifierAlignment: doesn't make so much sense

CVQualifierAlignment: Left
CVQualifierOrder: [ "inline", "static", "volatile", "const" ]

I'm leaning toward introducing <type> into the CVQualifierOrder allowing for some qualifiers to be Left of the type and some to be Right (this is not dissimilar to what was discussed some time ago, and similar to the concept of the resharper image above)

So we would be able to have:

CVQualifierOrder: [ "inline", "static", "volatile", "<type>", "const", "restrict" ]

By doing so inline,static,and volatile are Left and const,restrict are Right and so a single setting of Left and Right isn't applicable.

The whole feature could then be determined ONLY using the CVQualifierOrder however it doesn't feel like there is a good way to say "Don't change order" (unless of course I use an empty CVQualifierOrder list), but this doesn't feel very off by default enough, I like the explicitness of Leave.

My gut feeling is to make Left and Right options define a predetermined CVQualifierOrder, and introduce Custom as a new CVQualifierAlignment setting which would require you to define the Order, this has the advantage of driving a sort of semi standard as to what we mean by Left and Right

Left would by default define:

CVQualifierOrder: [ "inline", "static", "volatile", "const", "restrict", "<type>"]

Right would define:

CVQualifierOrder: [ "inline", "static", "<type>", "volatile", "const", "restrict" ]

(we might want to discuss what these defaults should be, I'm not a language lawyer!)

I realise "inline" and "static" are to the left in the Right alignment, but I'm not sure them being to the right is even legal (its certainly not common) or what is most likely wanted.

I also think if a CVQualifierOrder is defined and the CVQualifierAlignment is not Custom that should be a configuration error to ensure there isn't any ambiguity

Does anyone have any thoughts on this approach?

With the introduction of CVQualifierOrder the need for CVQualifierAlignment: doesn't make so much sense

CVQualifierAlignment: Left
CVQualifierOrder: [ "inline", "static", "volatile", "const" ]

I'm leaning toward introducing <type> into the CVQualifierOrder allowing for some qualifiers to be Left of the type and some to be Right (this is not dissimilar to what was discussed some time ago, and similar to the concept of the resharper image above)

So we would be able to have:

CVQualifierOrder: [ "inline", "static", "volatile", "<type>", "const", "restrict" ]

By doing so inline,static,and volatile are Left and const,restrict are Right and so a single setting of Left and Right isn't applicable.

The whole feature could then be determined ONLY using the CVQualifierOrder however it doesn't feel like there is a good way to say "Don't change order" (unless of course I use an empty CVQualifierOrder list), but this doesn't feel very off by default enough, I like the explicitness of Leave.

My gut feeling is to make Left and Right options define a predetermined CVQualifierOrder, and introduce Custom as a new CVQualifierAlignment setting which would require you to define the Order, this has the advantage of driving a sort of semi standard as to what we mean by Left and Right

Left would by default define:

CVQualifierOrder: [ "inline", "static", "volatile", "const", "restrict", "<type>"]

Right would define:

CVQualifierOrder: [ "inline", "static", "<type>", "volatile", "const", "restrict" ]

(we might want to discuss what these defaults should be, I'm not a language lawyer!)

I realise "inline" and "static" are to the left in the Right alignment, but I'm not sure them being to the right is even legal (its certainly not common) or what is most likely wanted.

I also think if a CVQualifierOrder is defined and the CVQualifierAlignment is not Custom that should be a configuration error to ensure there isn't any ambiguity

Does anyone have any thoughts on this approach?

Seems reasonable. I would like to remove the CV, only QualifierOrder. And please take attributes into account, C++ attributes like [[noreturn]], but also compiler attributes like __attribute__((noreturn)) or __declspec(noreturn), I think it should be possible to add <attributes> to your list.

clang/lib/Format/Format.cpp
1187

Braces :)

clang/lib/Format/QualifierAlignmentFixer.cpp
32 ↗(On Diff #365519)

Qualifier?

140 ↗(On Diff #365519)

More Braces (follow)

clang/lib/Format/QualifierAlignmentFixer.h
24 ↗(On Diff #365519)

I would go for only Qualifier.

I'm leaning toward introducing <type> into the CVQualifierOrder allowing for some qualifiers to be Left of the type and some to be Right (this is not dissimilar to what was discussed some time ago, and similar to the concept of the resharper image above)

Seems reasonable. I would like to remove the CV, only QualifierOrder. And please take attributes into account, C++ attributes like [[noreturn]], but also compiler attributes like __attribute__((noreturn)) or __declspec(noreturn), I think it should be possible to add <attributes> to your list.

I like both proposals.

clang/docs/ClangFormatStyleOptions.rst
1336

Why not just Qualifier to allow future additions for other qualifiers?
Technically, static or inline are not qualifiers, but that maybe is clear enough?

Other possibility, Specifier (as in "inline specifier")?
OTOH, Keyword may be too generic.

1342

Nit: cvqualifier seems ugly, here and below.

clang/lib/Format/Format.cpp
2446–2447

Nit: typos.

clang/lib/Format/QualifierAlignmentFixer.cpp
201 ↗(On Diff #365519)

That will access a nullptr if Next in the previous if was null. Is that possible BTW? Can we add a test for this? Maybe some possibly invalid code that starts has identifier and coloncolon but doesn't have a template opener? (maybe just const std::Foo)?

In D69764#2939037, @HazardyKnusperkeks wrote:

I would like to remove the CV, only QualifierOrder.

Oh gosh, I agree here so much with both you and @curdeius , I keep miss spelling the CV and it makes the code noisy... let me rework everything now to remove the CV including all the options,

I get @curdeius point about Specifier too... I'm OK about going with a concencus (I'm not married to the idea of just Qualifier)

And please take attributes into account, C++ attributes like [[noreturn]], but also compiler attributes like attribute((noreturn)) or __declspec(noreturn), I think it should be possible to add <attributes> to your list.

Oh man you are killing me with good suggestions ;-)....

So this will be hard with the current implementation because [ [ noreturn ] ] are all separate tokens. However maybe if we COULD merge the tokens into 1 token (like we do in FormatTokenLexer) even it it was just temporarily for this pass we maybe could do that.

I think I want to follow @klimek suggestion to give the QualifierAlignmentFixer its own set of proper unit tests to ensure I test its individual function at a lower unit level, rather than just looking at the output.

I'm in no rush, I want to let the RFC have a good amount time for people to read and to gather all the feedback, but I'm encouraged enough to think my efforts won't be wasted and I want to address some of @aaron.ballman concerns to extend a olive branch in the hope that I could at least partially gain approval.

It needs to wait for the next LLVM weekly in my view, where I'm thinking @asb might give it a mention, and then we should wait too for more feedback, as that could be a different audience.

So if you don't mind all the little and often updates (that's how I tend to work) I think its better I keep beating on this until everyone is happy.

If you are subscriber (as that list seems long) and don't want to listen to all the chatter I won't be offended if you drop off.

clang/docs/ClangFormatStyleOptions.rst
1342

I agree, I'm going to begin removal completely...

clang/lib/Format/QualifierAlignmentFixer.cpp
140 ↗(On Diff #365519)

Uh! this is my own style bleeding through... its why I need clang-format to remove them for me! ;-)

201 ↗(On Diff #365519)

I can't actually get this to produce a nullptr trying various

const std::Foo
const std::Foo<
const std::Foo<>
const std::Foo<int
const std::Foo<int>

I feel this is because its not going to be a TT_TemplateOpener if there isn't both < and > otherwise its a less than. I'll add some asserts to try and see if it ever happens

clang/lib/Format/QualifierAlignmentFixer.h
24 ↗(On Diff #365519)

Do you think everywhere now too? including the options?

With the introduction of CVQualifierOrder the need for CVQualifierAlignment: doesn't make so much sense

CVQualifierAlignment: Left
CVQualifierOrder: [ "inline", "static", "volatile", "const" ]

I'm leaning toward introducing <type> into the CVQualifierOrder allowing for some qualifiers to be Left of the type and some to be Right (this is not dissimilar to what was discussed some time ago, and similar to the concept of the resharper image above)

So we would be able to have:

CVQualifierOrder: [ "inline", "static", "volatile", "<type>", "const", "restrict" ]

By doing so inline,static,and volatile are Left and const,restrict are Right and so a single setting of Left and Right isn't applicable.

The whole feature could then be determined ONLY using the CVQualifierOrder however it doesn't feel like there is a good way to say "Don't change order" (unless of course I use an empty CVQualifierOrder list), but this doesn't feel very off by default enough, I like the explicitness of Leave.

My gut feeling is to make Left and Right options define a predetermined CVQualifierOrder, and introduce Custom as a new CVQualifierAlignment setting which would require you to define the Order, this has the advantage of driving a sort of semi standard as to what we mean by Left and Right

Left would by default define:

CVQualifierOrder: [ "inline", "static", "volatile", "const", "restrict", "<type>"]

Right would define:

CVQualifierOrder: [ "inline", "static", "<type>", "volatile", "const", "restrict" ]

(we might want to discuss what these defaults should be, I'm not a language lawyer!)

I realise "inline" and "static" are to the left in the Right alignment, but I'm not sure them being to the right is even legal (its certainly not common) or what is most likely wanted.

I also think if a CVQualifierOrder is defined and the CVQualifierAlignment is not Custom that should be a configuration error to ensure there isn't any ambiguity

Does anyone have any thoughts on this approach?

I think the general idea (letting the user specify the order of elements relative to one another rather than trying to come up with individual controls for each qualifier), I think that'll be easier for people to configure. I do worry about calling it "qualifier order" though as some of the things are definitely not qualifiers (like static or inline which are specifiers). A few questions about using an ordered list like this are: 0) should we diagnose typos (e.g., the user writes inlnie instead of inline)?, 1) should we diagnose duplications (e.g., the user lists friend twice)?

Btw, https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Sema/DeclSpec.h#L229 has quite a bit of information about the kinds of qualifiers and specifiers Clang deals with at the parsing stage, in case you were looking for a list of things to care about.

Oh man you are killing me with good suggestions ;-)....
So this will be hard with the current implementation because [ [ noreturn ] ] are all separate tokens. However maybe if we COULD merge the tokens into 1 token (like we do in FormatTokenLexer) even it it was just temporarily for this pass we maybe could do that.

I'd like to second the request for attribute support, even if it's follow-up work. Things like address space attributes, GC attributes, __unaligned, etc are all type qualifiers (I think we have a list of the attributes which are qualifiers in Type.h), so it's a natural extension. However, attributes in general are somewhat tricky because the attribute style (__attribute__ vs [[]]) can be syntactically location sensitive. e.g., [[noreturn]] void func() means something different from void [[noreturn]] func()` and we have some attributes that can appertain to a declaration or a type (so the positioning can have silent semantic changes).

I'm in no rush, I want to let the RFC have a good amount time for people to read and to gather all the feedback, but I'm encouraged enough to think my efforts won't be wasted and I want to address some of @aaron.ballman concerns to extend a olive branch in the hope that I could at least partially gain approval.

Not being in a rush sounds like a great plan! FWIW, I think we're moving steadily towards approval. I mentioned it on the RFC thread, but I'll mention it again here: thank you for starting the RFC process to have the broader discussion!

  • Remove the "CV" from the options
  • Add support for ordering static/inline/const/volatile around the type, allowing for mixing both left and right alignment at the same time
QualifierAlignment: Custom
QualifierOrder: [ "inline", "static", "volatile", "type", const ]
  • Add "Custom" format to allow use of QualifierOrder (may want to change the Qualifier name as its now both Qualifiers and Specifiers)
  • Add Parse errors and test to check for unsupported specifiers/qualifiers, duplicates,missing 'type' field and empty Order lists
  • Add Defaults for Left and Right Alignment (may need to discuss suitable defaults).
  • Removal of excess braces (if only clang-format could do that for us!!)
NOTE: Known issue:

Despite the clang-format file being already format correctly, I seem to be getting replacements: (e.g.)

For the following example

inline volatile int const c;

with the .clang-format file of:

---
Language: Cpp
BasedOnStyle: LLVM
QualifierAlignment: Custom
QualifierOrder: [ inline, static, volatile, type, const ]

I get the following:

$ clang-format -n test1.cpp
test1.cpp:1:1: warning: code should be clang-formatted [-Wclang-format-violations]
inline volatile int const c;
^

I believe caused by the existence of replacements that effectively do nothing.

$ clang-format test1.cpp --output-replacements-xml
<?xml version='1.0'?>
<replacements xml:space='preserve' incomplete_format='false'>
<replacement offset='0' length='15'>inline volatile</replacement>
</replacements>

Its likely I am switching the order from:

inline volatile int const c;

to

volatile inline int const c;

and then back to

inline volatile int const c;

Which I think likely becomes a single inline volatile replacement. This could in theory be a corner case for tooling::Replacements but I need to dig in more, its more likely an artefact of the reversing the LeftOrder rather keeping the Left order and not pushing specifiers though other specifiers that are more LeftMost combined with each specifier/qualifier being handled in its own pass.

I'll look into that next, but wanted to park the current implementation as functionally this is a significant step closer.

MyDeveloperDay marked 5 inline comments as done.Aug 11 2021, 9:11 AM
steveire added a comment.EditedSep 12 2021, 5:44 AM

FYI - there's nothing "anti-inclusive" about East/West.

In Qt I proposed a change which added properties to an object. Those properties in my change were named "Left"/"Right". The reviewer objected to those names because "scripts which are not Left-to-Right exist". So the reviewer required the names be changed to "First"/"Second". Note that the object and properties have nothing to do with text layout at all. It's very similar to the claim that East/West is somehow a marker of "Supremacy". Of course, even "First"/"Second" can be painted as "Supremacist" by someone sufficiently motivated.

Names shouldn't be chosen based on who makes claim to offense on behalf of some other group or population. If you decide to do that, then you can't use Left/Right either, nor First/Second.

Please restore East/West as options. People can use whichever the want in their config depending on what they are least offended by, if that's what they want to make decisions based on.

FYI - there's nothing "anti-inclusive" about East/West.

Thank you for your comment.

Personally I don't think that the question of not using East/West is whether these terms are inclusive or not.
It's more about being straight to the point, not multiplying multiple options (clang-format has lots of them already).
Moreover, Left/Right is IMO universally understood (we have left-to-right or right-to-left writing, left/right margin and so on, these words seem to be more popular in typography-related contexts).
East/West is on the other hand not that universal and often misunderstood. It also adds an unnecessary cognitive charge, because one needs to translate it to left/right in their head anyway.
Tangentially, the left part of my screen is not necessarily on the west...

My 2 cents.

FYI - there's nothing "anti-inclusive" about East/West.

While I'm certain that there can be confusion around why terms are or are not troublesome to some people and there needs to be space for those discussions, I'm also certain that telling someone that there is "nothing" to their concerns is not a good way to reach an understanding. Let's please be a bit more respectful when discussing concerns about using inclusive terminology (https://llvm.org/docs/CodeOfConduct.html#when-we-disagree-try-to-understand-why).

I think the relevance of Left/Right East/West as a setting is less important, as its more about an ordering, allowing some tokens to go to the Left and some to the Right of the "type"

QualifierAlignment: Custom
QualifierOrder: [ inline, static, type, const, volatile, restrict ]

While I currently include the Left/Right for some form of completeness I sort of feel i should kill them before we landing this, as I think even my defaults are a little subjective. (unless we can get concencus), hence I don't feel the need to bring back up the Left/Right/East/West debate. If its still a problem later we can add them.

if (Style.QualifierAlignment == FormatStyle::QAS_Right) {
      Style.QualifierOrder.clear();
      Style.QualifierOrder.push_back("type");
      Style.QualifierOrder.push_back("const");
      Style.QualifierOrder.push_back("volatile");
    } else if (Style.QualifierAlignment == FormatStyle::QAS_Left) {
      Style.QualifierOrder.clear();
      Style.QualifierOrder.push_back("const");
      Style.QualifierOrder.push_back("volatile");
      Style.QualifierOrder.push_back("type");
    }

QualifierAlignmentFixer need to process all the Left/Right passes internally before return the fixes on the original code. So now QualifierAlignmentFixer has its own inner set of passes which will be processed, and then "no op" fixes (replacements which make no change to the original code are removed (this was a problem before, as multiple passes causes changes that changed one way then changed it back "static const int a;" -> "const static int a;" -> "static const a;"

Move QualifierAlignmentFixer tests out to its own unit test file so Ultimately I can add more "Unit tests" on the functions

Still WIP but getting much closer.

I think the relevance of Left/Right East/West as a setting is less important, as its more about an ordering, allowing some tokens to go to the Left and some to the Right of the "type"

QualifierAlignment: Custom
QualifierOrder: [ inline, static, type, const, volatile, restrict ]

While I currently include the Left/Right for some form of completeness I sort of feel i should kill them before we landing this, as I think even my defaults are a little subjective. (unless we can get concencus), hence I don't feel the need to bring back up the Left/Right/East/West debate. If its still a problem later we can add them.

+1, I think this style of configuration is easier to reason about than left/right, east/west, before/after, etc because it's an explicit ordering.

Allow more QualifierFixer functions to be directly tested, remove some older commented code.

MyDeveloperDay marked 3 inline comments as done.Sep 14 2021, 1:29 AM
MyDeveloperDay added inline comments.
clang/docs/ClangFormatStyleOptions.rst
1336

If its ok I think we can stick with Qualifier although it does support static/inline/const/volatile/restrcit/constexpr I think the primary usage will be type const and const type

MyDeveloperDay marked an inline comment as done.
  • Resolve the final issue I had regarding overlapping replacements (was adding double spaces between keywords),
  • Add ability to add a warning into the documentation for modifying code. (as agreed via the RFC)

This now needs a reviewer and if we want to proceed someone has to be brave enough to accept it ;-)

Missed dump_format_style.py update

Remove debug code
Tidy a few comments
Remove Qualifier Order defaults (must be specified for Custom)

Really nice!

No attributes in there, do you think it would be difficult to add them? We can definitely do that in another change to move this one forward.

clang/include/clang/Format/Format.h
1146–1147

I would drop that use with caution. I think without the warning is big enough, and with it's too much.

clang/lib/Format/Format.cpp
2414

Nit: Unrelated (and unnecessary) change.

clang/lib/Format/QualifierAlignmentFixer.cpp
28 ↗(On Diff #373657)

Out of curiosity, on what premise do you choose a static member function that is only used in this file or a local free function? I would always choose the latter (with an anon namespace), to keep the header smaller.

57 ↗(On Diff #373657)

Does not need to be addressed here, but does LLVM have a string builder or so? This produces a lot of (de-)allocations.

129 ↗(On Diff #373657)

I would prefer to match the order of functions in the header with the order in the cpp.

161 ↗(On Diff #373657)

This name is legacy from just const volatile formatting?

347 ↗(On Diff #373657)

Only else and assert? It does nothing if Alignment is something different, or?

366 ↗(On Diff #373657)

Couldn't you just

LeftOrder.assign(std::reverse_iterator(type) /*maybe with a next or prev, haven't thought too much about it*/, Order.rend());
RightOrder.assign(std::next(type), Order.end());

?

clang/lib/Format/QualifierAlignmentFixer.h
24 ↗(On Diff #373657)

I don't know what the LLVM style is on that, but I prefer using anytime over typedef.

30 ↗(On Diff #373657)

Has the 8 some meaning? Then maybe give it a name. Or is it just coincidence that you repeat the 8 for QualifierTokens?

clang/unittests/Format/FormatTest.cpp
12510

Unused?

MyDeveloperDay marked 11 inline comments as done.

Address review comments.

  • reorder functions to match header

Fix an overlapping replacement issue when going from Right to Left (just jump past the token once done)

Allow the use of "TypenameMacros" to allow for Capitialized types (be aware they should be non pointer types and don't have to be macros, so may need its own future "NonPtrTypes" setting)

MyDeveloperDay added inline comments.Sep 21 2021, 1:38 AM
clang/lib/Format/QualifierAlignmentFixer.cpp
28 ↗(On Diff #373657)

I've been pulled up on using anon namespaces before in previous reviews, personally I always use them myself in my code. To be honest I've been moving most of these functions into actual class statics functions so I can test the functions explicitly in the unit tests (anon namespaces aren't good for that obvs)

I'd like to leave them as statics so I can more easily do that move and add more tests if I find issues.

57 ↗(On Diff #373657)

I'm not a massive fan of this either, but I'm unsure if LLVM has anything, I think getting functional first is important, we can go for fast if someone can point out a better pattern.

129 ↗(On Diff #373657)

I can spin that around, (its going to mess with review comments, but I know what you mean)

347 ↗(On Diff #373657)

You know this was my bad because I reused the alignment type because it had the left and right but this could be a boolean as its different from the QualifierAlignment, let me change this for clarity

366 ↗(On Diff #373657)

There is probably a better way but it needs to handle "type" being the first or last element, I tried and it didn't quite work.

clang/lib/Format/QualifierAlignmentFixer.h
24 ↗(On Diff #373657)

I was actually just following what is in Format.cpp

30 ↗(On Diff #373657)

For SmallVector this is basically a "ShortStringOptimization" for vector i.e. its stack allocated until the vector goes over 8, I have 7 qualifiers, and I wanted it an order of 2 so hence 8 (its shouldn't grow (and heap allocation) unless we define more qualifier types (this only supports what I say for now)

I think the use of a literal is quite common in this case, its really just a hint, I think its ok to use without it being a variable.

If we don't specify types in the QualifierOrder (say static or inline as in default Left/Right) then don't push const through them just because they can be a type of qualifier we support.

i.e. only push through what we specify, ultimately this helps reduce the number of changes (caused by inline/static) being in different orders (perhaps unintentional) but where you don't care (i.e. you haven't specified them)

Its no longer essential to compare the fixes against the original code, as this was needed because of an artifact with adding double spaces and was resolved with checking for if the previous token already ended or started with a space, (this speeds things up a little).

Added a unit test to validate that const splitting unsigned and char correctly resolves.

clang/lib/Format/QualifierAlignmentFixer.cpp
57 ↗(On Diff #373657)

Full support for that.

clang/lib/Format/QualifierAlignmentFixer.h
30 ↗(On Diff #373657)

I know what it is. So the 8 is NumberOfSupportedQualifiers? My point is that if we add something, let's say attributes ;), one only need to change that constant and both vectors grow accordingly, so that no heap allocation happens.

Nothing I would block that change for.

clang/unittests/Format/QualifierFixerTest.cpp
573 ↗(On Diff #373954)

This one would suffice, since we add the values within the test now. But actually I would go for Style.QualifierOrder = {"inline",...}; and no EXPECTs needed.

855 ↗(On Diff #373954)

This is not correct, the documentation of TypenameMacros says

These are expected to be macros of the form:

STACK_OF(...)

and HRESULT is just a typedef.
So a test against STACK_OF(int) is useful. But the macro detection should be reworked, or maybe dropped?
The handling of LLVM_NODISARD basically boils down to handling attributes and AttributeMacros.

IdanHo added a subscriber: IdanHo.Sep 22 2021, 10:52 AM
MyDeveloperDay marked 3 inline comments as done.

Remove use of Typenamemacros (we'll solve this a different way later)

Use better vector initialization

clang/lib/Format/QualifierAlignmentFixer.cpp
449 ↗(On Diff #373954)

I think this is still right, because it is a type (according to the configuration).

clang/lib/Format/QualifierAlignmentFixer.h
16 ↗(On Diff #374298)

This clang-tidy isn't addressed.

clang/unittests/Format/QualifierFixerTest.cpp
122 ↗(On Diff #374298)

In addition to clang-tidies message: It is unnecessary.

MyDeveloperDay marked 3 inline comments as done.

Fix clang-format and clang-tidy issues

clang/lib/Format/QualifierAlignmentFixer.cpp
449 ↗(On Diff #373954)

yeah, let's do this a different way after we have landed this, I was thinking about having a list of "Types" that users could specify that would help us classify tok::identifiers a little better (could help with some of the casting issues we've seen before)

I notice in projects like Linux they make heavy use of these ForeachMacros and StatementMacros, I don't want to steal another list I'd rather we had one that was very specific to types, I think this could help

Let's give it a first shot. When it has landed maybe I find the time to look into the attributes. ;)

clang/lib/Format/QualifierAlignmentFixer.cpp
449 ↗(On Diff #373954)

We can do that.

This revision is now accepted and ready to land.Sep 23 2021, 11:23 AM

If there are no other objections I'm going to land this shortly.

This revision was landed with ongoing or failed builds.Sep 23 2021, 12:01 PM
This revision was automatically updated to reflect the committed changes.

This broke buildbots that have -Werror specified. I pushed in a fix in https://reviews.llvm.org/rG76d845cb169f048cb6f2176c3e7a6534dc5af097.
Also, please consider formatting your commit messages to prevent wrapping.

This broke buildbots that have -Werror specified. I pushed in a fix in https://reviews.llvm.org/rG76d845cb169f048cb6f2176c3e7a6534dc5af097.
Also, please consider formatting your commit messages to prevent wrapping.

Ok perfect, thank you and thanks for the advice. I'll try turning on -Werror in my local builds

simon.giesecke added inline comments.
clang/docs/ClangFormatStyleOptions.rst
2069

This is pretty unclear, for a number of reasons:

  • First, this probably only refers to a setting other than QAS_Leave?
  • Second, "lead to incorrect code generation" seems to skip a step. In the first place, this seems to imply that a setting other than QAS_Leave might change the semantics of the source code.
  • Third, it's not clear to me when this would happen, and how likely that is. Does this mean "Non-default settings are experimental, and you shouldn't use this in production?" or rather "Under rare circumstances that are unlikely to happen in real code, a non-default setting might change semantics." At the minimum, could this give some example(s) when this happens?
clang/docs/ClangFormatStyleOptions.rst
2069
  • Yes.
  • Yes, it might change the semantics, that was the content of a huge discussion here in the review and on the mailing list. Consensus was found that non whitespace changes are acceptable with a big warning and off by default.
  • The latter, if we would have an example at hand in which cases it wouldn't work, we would fix that and not give it as an example. So to the best of our knowledge it does not break anything.

The warning text however might be improved.

MyDeveloperDay marked 2 inline comments as done.Sep 29 2021, 3:06 PM
MyDeveloperDay added inline comments.
clang/docs/ClangFormatStyleOptions.rst
2069

Agreed its tough to give a meaningful example that actually currently breaks code, but the example I was given was

#define INTPTR int *

const INTPTR a;

In this sense moving const changes this from

const int * a;

to

int * const a;

For this we overcome this by NOT supporting "UPPERCASE" identifiers incase they are macros, we have plans to add support for identifying "type identifiers"

I guess if someone does

#define intptr int *

Then this could go still go wrong, however I like to think that these examples are on the "edge" of good code. (should we pander to what could be considered bad style in the first place?)

The warning was a concession to those who felt its should be highlighted as an option that could change behaviour (like SortIncludes was famously identified), and as @HazardyKnusperkeks say, we are looking for people to tell us where this breaks so we can try and fix it. (macros are already an issue for clang-format the solution for which is clang-format off/on)

I personally feel the no need to elaborate further on the warning at this time, but I'm happy to support someone if they feel they want to extend it.

If you are concerned my advice is don't use this option. But clang-format can be used in an advisor capacity (with -n or --dry-run) and this can be used to notify cases of incorrect qualifier ordering without actually using it to change the code.

simon.giesecke added inline comments.Sep 30 2021, 1:00 AM
clang/docs/ClangFormatStyleOptions.rst
2069

Thanks a lot for the explanation. Sorry I hadn't read through the entire review discussion. I saw the comment in the generated documentation and then dug up this patch that introduced it.

I see how macros might break this (I think I ran into a similar problem when trying to fix the const alignment manually using some sed machinery, though luckily it broke the build and didn't end up in bad code generation). It would be good to understand if there are any cases not involving macros whose semantics are modified. Identifying macros via the naming convention can't be reliable. Even if one did that consistently in their own code base, chances are high that library headers don't follow the same conventions. And yes, probably cases where a qualifier is added via a macro is not good style anyway. I replaced that by std::add_const_t in the mentioned case. But the use cases of clang-format are probably not confined to code written in a good style.

I think it's good to extend the warning a bit, otherwise anyone reading it would need to dig up this patch and read through the review discussion to judge it. I'll leave a comment on D110801 as well.

If you are concerned my advice is don't use this option.

Well, I think if it works "reliably", it's very useful. I was searching for a way to harmonize the const/qualifier alignment style, and I first thought that would be clang-tidy land, and then found this clang-format patch. And I guess, maybe that's exactly the gap that leads to this problem: In clang-tidy, there would be the extra semantic information (we know what's a macro or type etc.) that would allow to prevent any semantic changes.