[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Erich Keane via Phabricator via cfe-commits]

erichkeane resigned from this revision.
erichkeane added a comment.
This revision now requires review to proceed.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D69764#2935269 , @erichkeane wrote:

> In D69764#2935218 , @MyDeveloperDay 
> wrote:
>
>> In D69764#2934666 , @erichkeane 
>> wrote:
>>
 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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D69764#2935218 , @MyDeveloperDay 
wrote:

> In D69764#2934666 , @erichkeane 
> wrote:
>
>>> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D69764#2934666 , @erichkeane wrote:

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D69764#2935036 , @klimek wrote:

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In D69764#2934686 , @aaron.ballman 
wrote:

> In D69764#2934612 , @MyDeveloperDay 
> wrote:
>
>>> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D69764#2934612 , @MyDeveloperDay 
wrote:

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D69764#2934646 , @MyDeveloperDay 
wrote:

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.



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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.



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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D69764#2934560 , @MyDeveloperDay 
wrote:

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D69764#2934535 , @MyDeveloperDay 
wrote:

>> 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).
>
> 2. 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.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

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

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

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D69764#2934489 , @MyDeveloperDay 
wrote:

> In D69764#2934378 , @erichkeane 
> wrote:
>
>> 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?
>
> F18452578: image.png 
>
> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D69764#2934483 , @erichkeane wrote:

> In D69764#2934473 , @MyDeveloperDay 
> wrote:
>
>> In D69764#2934378 , @erichkeane 
>> wrote:
>>
>>> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D69764#2934378 , @erichkeane wrote:

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

F18452578: image.png 

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D69764#2934473 , @MyDeveloperDay 
wrote:

> In D69764#2934378 , @erichkeane 
> wrote:
>
>> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D69764#2934378 , @erichkeane wrote:

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Erich Keane via Phabricator via cfe-commits]

erichkeane requested changes to this revision.
erichkeane added a comment.
This revision now requires changes to proceed.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D69764#2934124 , @klimek wrote:

> In D69764#2934032 , @MyDeveloperDay 
> wrote:
>
>> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In D69764#2934032 , @MyDeveloperDay 
wrote:

> In D69764#2932929 , @steveire wrote:
>
>> 
>
> @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 :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added subscribers: djasper, klimek.
MyDeveloperDay added a comment.

In D69764#2932929 , @steveire wrote:

> 

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

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

2.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D69764#2877618 , @atomgalaxy wrote:

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D69764#2876916 , @MyDeveloperDay 
wrote:

>> 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
>
> F17930223: image.png 

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

In D69764#2877614 , @MyDeveloperDay 
wrote:

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Gašper Ažman via cfe-commits]

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.

On Wed, Jul 14, 2021 at 6:42 PM MyDeveloperDay via Phabricator <
revi...@reviews.llvm.org> wrote:

> MyDeveloperDay added a comment.
>
> 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)
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D69764/new/
>
> https://reviews.llvm.org/D69764
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

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)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Gašper Ažman via cfe-commits]

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

On Wed, Jul 14, 2021 at 1:47 PM Marek Kurdej via Phabricator <
revi...@reviews.llvm.org> wrote:

> curdeius added a comment.
>
> 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.
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D69764/new/
>
> https://reviews.llvm.org/D69764
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

> 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 not putting more effort into improving 
the current design of this patch to handle more cases 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

F17930223: image.png 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Gašper Ažman via cfe-commits]

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.

On Tue, Jul 13, 2021 at 6:03 PM Aaron Ballman via Phabricator <
revi...@reviews.llvm.org> wrote:

> aaron.ballman added a comment.
>
> In D69764#2874404 ,
> @MyDeveloperDay wrote:
>
> >> What you're describing sounds like clang-tidy but perhaps with improved
> support for composing source modifications from fix-its, or do you envision
> something rather different?
> >
> > All my uses of clang-tidy require extensive command line arguments to
> handle compiler flags and include paths, I don't want that for this when
> its mostly unnecessary:
> >
> >file.cxx
> >
> > is all that should be required.
>
> 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.
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D69764/new/
>
> https://reviews.llvm.org/D69764
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Mateusz Furdyna via Phabricator via cfe-commits]

furdyna added a comment.

> 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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

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.




Comment at: clang/docs/ClangFormatStyleOptions.rst:1378
 
+**ConstStyle** (``ConstAlignmentStyle``)
+  Different ways to arrange const.

davidstone wrote:
> steveire wrote:
> > klimek wrote:
> > > MyDeveloperDay wrote:
> > > > aaron.ballman wrote:
> > > > > MyDeveloperDay wrote:
> > > > > > klimek wrote:
> > > > > > > Personally, I'm somewhat against having 3 different aliases for 
> > > > > > > the options. I'd chose one, even though it doesn't make everybody 
> > > > > > > happy, and move on. I'm fine with East/West as long as the 
> > > > > > > documentation makes it clear what it is.
> > > > > > If I have to drop the other options, I think I'd want to go with 
> > > > > > East/West const as I feel it has more momentum, just letting people 
> > > > > > know before I change the code back (to my original patch ;-) )
> > > > > > 
> > > > > > https://www.youtube.com/watch?v=gRmI_gsNqcI
> > > > > > 
> > > > > > {F10954065}
> > > > > > 
> > > > > @klimek I requested that we do not go with East/West the options and 
> > > > > I'm still pretty insistent on it. East/West is a kitschy way to 
> > > > > phrase it that I think is somewhat US-centric (where we make a pretty 
> > > > > big distinction between the east and west coasts). I do not want to 
> > > > > have to mentally map left/right to the less-clear east/west in the 
> > > > > config file. Would you be fine if we only had Left/Right instead of 
> > > > > East/West? I would be fine with that option, but figured enough 
> > > > > people like the cute East/West designation that we might as well 
> > > > > support it.
> > > > Just for a reference, I'm not from the US and I think east/west still 
> > > > translates pretty well. I was happy to support the others. 
> > > I'd be fine with only having left/right; my personal feeling is also that 
> > > west-const / east-const has kinda become a term of art, though, so I 
> > > really don't know which one is "right" :)
> > > 
> > > Generally, I think this is one of the cases where, given good docs, we're 
> > > quickly spending more engineering hours discussing the right solution 
> > > than it'll cost aggregated over all future users, under the assumption 
> > > that people do not write new configs very often, and the few who will, 
> > > will quickly remember.
> > > 
> > > I'd be fine with only having left/right; my personal feeling is also that 
> > > west-const / east-const has kinda become a term of art, though, so I 
> > > really don't know which one is "right" :)
> > 
> > This reminds me of the joke that Americans drive on the "Right" side of the 
> > road, and English drive on the "Correct" side. Sort of gives a different 
> > meaning to `ConstStyle : Right` and that the alternative is `Wrong` :). 
> > Maybe that language ambiguity is why `East`/`West` caught on.
> > 
> > > people do not write new configs very often
> > 
> > Agreed. It seems a small number of strong views might influence this enough 
> > to make `East`/`West` not be used. What a pity.
> I'd also add that the original "joke" was `const west` vs `east const` -- the 
> terms put the const where they would apply in the style. This seems to have 
> gotten lost in the retelling and now the const is always on the right.
> 
> I'd favor `Left` and `Right`. `East` and `West` have "caught on" now, but 
> it's caught on among the small group of C++ enthusiasts and still has a sort 
> of ingroup joke feel to it. I instinctively know my preferred style is "east" 
> because the phrase "east const" feels more natural to me because I've said it 
> more. It took me a bit of repetition of thinking "East is right, I put the 
> const on the right" to get there.
> 
> `East` and `West` also sets a precedent for future options to refer to 
> positions as `North` and `South`.
I also think `Left` and `Right` are the better choices, but I would be full in 
favor of also parsing `West` and `East`. :)



Comment at: clang/lib/Format/EastWestConstFixer.cpp:293
+  Tok->Next->Next->Next && Tok->Next->Next->Next->is(tok::kw_const)) {
+swapFourTokens(SourceMgr, Fixes, Tok, Tok->Next, Tok->Next, 
Tok->Next->Next->Next,
+ /*West=*/true);

aaron.ballman wrote:
> curdeius wrote:
> > 

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D69764#2874790 , @MyDeveloperDay 
wrote:

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D69764#2874464 , @atomgalaxy wrote:

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



In D69764#2874514 , @MyDeveloperDay 
wrote:

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

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

I honestly don't deny that a const fixer could be added to clang-tidy and that 
would be great, but for the same reason that I think that the 
braces-around-statements clang-tidy check isn't necessarily the only way to 
work.

I've used braces-around-statements in clang-tidy and trying to run that 
repeated over a very large project is painful.

You asked for both fast and correct, lets me honest clang-tidy can very often 
neither of those things and whilst is can be made correct, really is always 
going to struggle being fast. This turns a subsecond reformatting (that may 
need to do nothing) into a multi minute build, and whilst I love it for what is 
can do, for some of these simple tasks of which the expectation is to do almost 
nothing, its a massive overkill.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D69764#2874404 , @MyDeveloperDay 
wrote:

>> What you're describing sounds like clang-tidy but perhaps with improved 
>> support for composing source modifications from fix-its, or do you envision 
>> something rather different?
>
> All my uses of clang-tidy require extensive command line arguments to handle 
> compiler flags and include paths, I don't want that for this when its mostly 
> unnecessary:
>
>file.cxx 
>
> is all that should be required.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.



> What you're describing sounds like clang-tidy but perhaps with improved 
> support for composing source modifications from fix-its, or do you envision 
> something rather different?

All my uses of clang-tidy require extensive command line arguments to handle 
compiler flags and include paths, I don't want that for this when its mostly 
unnecessary:

   file.cxx 

is all that should be required.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D69764#2873225 , @MyDeveloperDay 
wrote:

> If this is something we think we wouldn't want in clang-format, what about 
> the idea of building a new binary which did allow the modification of the 
> tokens, This comes up from time to time and I kind of feel its a weak 
> argument for those that don't want to use it enforcing their will on those 
> that do.
>
> I feel we have D69764: [clang-format] Add East/West Const fixer capability 
>  and D95168: [clang-format] Add InsertBraces 
> option  both of which seem to break the 
> fundamental belief that some feel that we shouldn't change anything other 
> than whitespace (A fact that has already been broken 3 times already).
>
> But really these are just separate passes, so why couldn't clang-format allow 
> custom passes to be added much like any compiler does.
>
> If the objection is that this isn't clang-format's role then what if we build 
> something new whose purpose IS to allow these kinds of changes?

What you're describing sounds like clang-tidy but perhaps with improved support 
for composing source modifications from fix-its, or do you envision something 
rather different?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

If this is something we think we wouldn't want in clang-format, what about the 
idea of building a new binary which did allow the modification of the tokens, 
This comes up from time to time and I kind of feel its a weak argument for 
those that don't want to use it enforcing their will on those that do.

I feel we have D69764: [clang-format] Add East/West Const fixer capability 
 and D95168: [clang-format] Add InsertBraces 
option  both of which seem to break the 
fundamental belief that some feel that we shouldn't change anything other than 
whitespace (A fact that has already been broken 3 times already).

But really these are just separate passes, so why couldn't clang-format allow 
custom passes to be added much like any compiler does.

If the objection is that this isn't clang-format's role then what if we build 
something new whose purpose IS to allow these kinds of changes?

I'm pretty rubbish with names but what about:

  clang-format++
  clang-modifier
  clang-rewriter

I think it still has some value to those that want to use it, and I'd like to 
find an elegant way to deliver it.

  if (Style.Language == FormatStyle::LK_Cpp) {
if (Style.FixNamespaceComments)
  Passes.emplace_back([&](const Environment ) {
return NamespaceEndCommentsFixer(Env, Expanded).process();
  });
  
if (Style.SortUsingDeclarations)
  Passes.emplace_back([&](const Environment ) {
return UsingDeclarationsSorter(Env, Expanded).process();
  });
  
if (Style.InsertBraces != FormatStyle::BIS_Never)
  Passes.emplace_back([&](const Environment ) {
return BracesInserter(Env, Expanded).process();
  });
  
if (Style.isCpp() && Style.ConstPlacement != FormatStyle::CS_Leave)
Passes.emplace_back([&](const Environment ) {
  return EastWestConstFixer(Env, Expanded).process();
});
  }
  
  if (Style.Language == FormatStyle::LK_JavaScript &&
  Style.JavaScriptQuotes != FormatStyle::JSQS_Leave)
Passes.emplace_back([&](const Environment ) {
  return JavaScriptRequoter(Env, Expanded).process();
});
  
  Passes.emplace_back([&](const Environment ) {
return Formatter(Env, Expanded, Status).process();
  });
  
  if (Style.Language == FormatStyle::LK_JavaScript &&
  Style.InsertTrailingCommas == FormatStyle::TCS_Wrapped)
Passes.emplace_back([&](const Environment ) {
  return TrailingCommaInserter(Env, Expanded).process();
});


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

> Should any known failure modes be documented?

At present, I don't know of any, I think I could probably trick it with macros 
but then that is #clang-format  all 
over,

if you are using it feel free to log bugs in the bug tracker and add a 
reference to D69764  if you see any and I'll 
take a look.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D69764#2867109 , @MyDeveloperDay 
wrote:

> In D69764#2863648 , @owenpan wrote:
>
>> Has this been tested against a large code base? It also needs an unqualified 
>> LGTM before it can be merged.
>
> D105701: [clang-format] test revision (NOT FOR COMMIT) to demonstrate 
> east/west const fixer capability  
> demonstrates transforming clang-format itself to east const.
>
> Actually transformation of the whole of the clang subfolder is actually 
> holding up pretty well. I'm not seeing an violations (not sure if I 
> transformed all the files) but certainly so much so that creating a review 
> that covered all of it was way too big.
>
> Testing on a large code base can be hard especially one as large as LLVM 
> where its not currently fully clang-formatted in the first place.
>
> Of course the lit tests get mangled as the test code gets swapped but the 
> //CHECK-FIXES doesn't
>
> Like I mentioned before, by gut feeling is that this option is MOST useful in 
> preventing violation to your current style from creeping in than going to the 
> extreme of transforming a whole project from east to west or vice versa.

FYI - I also tested this on a large codebase at work. It is now used to keep 
the current style as you describe.

Should any known failure modes be documented?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D69764#2863648 , @owenpan wrote:

> Has this been tested against a large code base? It also needs an unqualified 
> LGTM before it can be merged.

D105701: [clang-format] test revision (NOT FOR COMMIT) to demonstrate east/west 
const fixer capability  demonstrates 
transforming clang-format itself to east const.

Actually transformation of the whole of the clang subfolder is actually holding 
up pretty well. I'm not seeing an violations (not sure if I transformed all the 
files) but certainly so much so that creating a review that covered all of it 
was way too big.

Testing on a large code base can be hard especially one as large as LLVM where 
its not currently fully clang-formatted in the first place.

Of course the lit tests get mangled as the test code gets swapped but the 
//CHECK-FIXES doesn't

Like I mentioned before, by gut feeling is that this option is MOST useful in 
preventing violation to your current style from creeping in than going to the 
extreme of transforming a whole project from east to west or vice versa.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

To be more sure of not breaking something we'd likely have to reduce the cases 
where tok::identifier was checked, it depends if "not catching every case" is 
caught is more acceptable. I certainly see that elsewhere in clang (like 
identifying where override is missing)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D69764#2863312 , @steveire wrote:

> In D69764#2863266 , @aaron.ballman 
> wrote:
>
>> In D69764#2863213 , @steveire wrote:
>>
>>> @MyDeveloperDay  Does anything prevent this being merged, instead of just 
>>> rebased?
>>
>> Please see my comments from https://reviews.llvm.org/D69764#2533538.
>
> A clang-tidy check will never be as fast as clang-query.
>
> Anyway, you're against merging this, but there are clearly many people who 
> prefer to see it merged. Can we merge it?

No. My concerns are with the fact that this feature can break working code and 
there's significant value in being able to trust your code formatting tool will 
not do that. That sentiment has been expressed by others as a matter of concern 
for them as well.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

FYI, I'm not actually asking for this to be merged, its just I'm happy to keep 
rebasing it from time to time for others to use,  but to do that I have to keep 
pushing new reviews, but if the general opinion was to put it in then I'd go 
for that.

I knew it was going to be a contentious idea, and I think its very hard to do 
100% correctly without semantic information (like lots of other aspects of 
#clang-format ), because of that 
I'm a little nervous, but as its off by default I'd hope we could mitigate that.

I personally just don't want it for clang-tidy, as as much as I like clang-tidy 
I feel less that 1% of the people using clang-format are using clang-tidy 
because of the issues of setting something up to actually run it, and for 
something with semantic information clang-tidy -fix regularly butchers code 
making it incorrect,  just saying...  clang-format too is far from perfect, if 
you live on the edge of what I would consider to be sensible readable coding, 
it will mutate it into something that won't compile especially if you have an 
over propensity to use macros!

My feelings for this are a little mixed, when I started it only handled a few 
cases and I felt it was more robust, as people pushed me to support more cases 
I've become less happy that it won't break their code during correction.

Whilst this may not be suitable for mainstream clang-format, I think this makes 
for a great CI tool that is being used to "check" code rather than "correct" 
code, and so I build this for my CI system so I can catch people adding "east 
const" where I don't want them to. But for my large project I don't proactively 
flip back and forth between west and east to ensure I'm not making the code 
incorrect.

I run `clang-format -n` on my nightly & CI builds to pull out clang-format 
issues as build warnings in the log file, this means I can turn clang-format 
failures into notifications which get fixed almost immediately. People forget 
to run clang-format  way more often than they put in "east const", most of my 
developers have developed the muscle memory to not do that, but this helps 
bring new developers onboard without me having to crush their dreams during 
code review!

When it comes to "Finding" east/west const violations this does the job and I 
am unaware of any other tool that does that in the entire C++ ecosystem, for 
that point alone I think having such a tool is worth it. I wouldn't want to put 
this into any other tool because of the clang-format integration points being 
already established with editors/CI systems etc...  (the exact same reasons why 
I wanted C# and JSON formatting)

My 2p worth


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

Has this been tested against a large code base? It also needs an unqualified 
LGTM before it can be merged.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D69764#2863266 , @aaron.ballman 
wrote:

> In D69764#2863213 , @steveire wrote:
>
>> @MyDeveloperDay  Does anything prevent this being merged, instead of just 
>> rebased?
>
> Please see my comments from https://reviews.llvm.org/D69764#2533538.

A clang-tidy check will never be as fast as clang-query.

Anyway, you're against merging this, but there are clearly many people who 
prefer to see it merged. Can we merge it?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D69764#2863213 , @steveire wrote:

> @MyDeveloperDay  Does anything prevent this being merged, instead of just 
> rebased?

Please see my comments from https://reviews.llvm.org/D69764#2533538.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

@MyDeveloperDay  Does anything prevent this being merged, instead of just 
rebased?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D69764#2858656 , @MyDeveloperDay 
wrote:

> Rebase on clang12 as requested

Thank you!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 356565.
MyDeveloperDay removed reviewers: klimek, djasper.
MyDeveloperDay added a comment.

Rebase on clang12 as requested


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/EastWestConstFixer.cpp
  clang/lib/Format/EastWestConstFixer.h
  clang/lib/Format/Format.cpp
  clang/tools/clang-format/ClangFormat.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -18202,6 +18202,13 @@
   CHECK_PARSE("ContinuationIndentWidth: 11", ContinuationIndentWidth, 11u);
   CHECK_PARSE("CommentPragmas: '// abc$'", CommentPragmas, "// abc$");
 
+  Style.ConstPlacement = FormatStyle::CS_West;
+  CHECK_PARSE("ConstPlacement: Leave", ConstPlacement, FormatStyle::CS_Leave);
+  CHECK_PARSE("ConstPlacement: East", ConstPlacement, FormatStyle::CS_East);
+  CHECK_PARSE("ConstPlacement: West", ConstPlacement, FormatStyle::CS_West);
+  CHECK_PARSE("ConstPlacement: Right", ConstPlacement, FormatStyle::CS_East);
+  CHECK_PARSE("ConstPlacement: Left", ConstPlacement, FormatStyle::CS_West);
+
   Style.AlignConsecutiveAssignments = FormatStyle::ACS_Consecutive;
   CHECK_PARSE("AlignConsecutiveAssignments: None", AlignConsecutiveAssignments,
   FormatStyle::ACS_None);
@@ -22146,6 +22153,330 @@
   "}";
   EXPECT_EQ(Code, format(Code, Style));
 }
+
+TEST_F(FormatTest, EastWestConst) {
+  FormatStyle Style = getLLVMStyle();
+
+  // keep the const style unaltered
+  verifyFormat("const int a;", Style);
+  verifyFormat("const int *a;", Style);
+  verifyFormat("const int ", Style);
+  verifyFormat("const int &", Style);
+  verifyFormat("int const b;", Style);
+  verifyFormat("int const *b;", Style);
+  verifyFormat("int const ", Style);
+  verifyFormat("int const &", Style);
+  verifyFormat("int const *b const;", Style);
+  verifyFormat("int *const c;", Style);
+
+  verifyFormat("const Foo a;", Style);
+  verifyFormat("const Foo *a;", Style);
+  verifyFormat("const Foo ", Style);
+  verifyFormat("const Foo &", Style);
+  verifyFormat("Foo const b;", Style);
+  verifyFormat("Foo const *b;", Style);
+  verifyFormat("Foo const ", Style);
+  verifyFormat("Foo const &", Style);
+  verifyFormat("Foo const *b const;", Style);
+
+  verifyFormat("LLVM_NODISCARD const int ();", Style);
+  verifyFormat("LLVM_NODISCARD int const ();", Style);
+
+  verifyFormat("volatile const int *restrict;", Style);
+  verifyFormat("const volatile int *restrict;", Style);
+  verifyFormat("const int volatile *restrict;", Style);
+}
+
+TEST_F(FormatTest, EastConst) {
+  FormatStyle Style = getLLVMStyle();
+  Style.ConstPlacement = FormatStyle::CS_East;
+
+  verifyFormat("int const a;", Style);
+  verifyFormat("int const *a;", Style);
+  verifyFormat("int const ", Style);
+  verifyFormat("int const &", Style);
+  verifyFormat("int const b;", Style);
+  verifyFormat("int const *b;", Style);
+  verifyFormat("int const ", Style);
+  verifyFormat("int const &", Style);
+  verifyFormat("int const *b const;", Style);
+  verifyFormat("int *const c;", Style);
+
+  verifyFormat("Foo const a;", Style);
+  verifyFormat("Foo const *a;", Style);
+  verifyFormat("Foo const ", Style);
+  verifyFormat("Foo const &", Style);
+  verifyFormat("Foo const b;", Style);
+  verifyFormat("Foo const *b;", Style);
+  verifyFormat("Foo const ", Style);
+  verifyFormat("Foo const &", Style);
+  verifyFormat("Foo const *b const;", Style);
+  verifyFormat("Foo *const b;", Style);
+  verifyFormat("Foo const *const b;", Style);
+  verifyFormat("auto const v = get_value();", Style);
+  verifyFormat("long long const ", Style);
+  verifyFormat("unsigned char const *a;", Style);
+  verifyFormat("int main(int const argc, char const *const *const argv)",
+   Style);
+
+  verifyFormat("LLVM_NODISCARD int const ();", Style);
+  verifyFormat("SourceRange getSourceRange() const override LLVM_READONLY",
+   Style);
+  verifyFormat("void foo() const override;", Style);
+  verifyFormat("void foo() const override LLVM_READONLY;", Style);
+  verifyFormat("void foo() const final;", Style);
+  verifyFormat("void foo() const final LLVM_READONLY;", Style);
+  verifyFormat("void foo() const LLVM_READONLY;", Style);
+
+  verifyFormat(
+  "template  explicit Action(Action const );",
+  Style);
+  verifyFormat(
+  "template  explicit Action(Action const );",
+  "template  explicit Action(const Action& action);",
+  Style);
+  verifyFormat(
+  "template  explicit Action(Action const );",
+  "template \nexplicit Action(const Action& action);",
+  Style);
+
+  verifyFormat("int const a;", "const int a;", Style);
+  

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

Is it possible to have this rebased on top of LLVM12 for those of us who find 
the trade-offs acceptable? I prefer a consistent formatting of const placement, 
and my code base does not have issues with the macros - if we find one that 
break the build, I'll suggest it here as a test case ;)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[David Stone via Phabricator via cfe-commits]

davidstone added a comment.

In D69764#2058590 , @rsmith wrote:

> I think that if we are reordering `const`, we should be reordering all 
> //decl-specifier//s -- I'd like to see `int static constexpr unsigned const 
> long inline` reordered to something like `static constexpr inline const 
> unsigned long int` too. Applying this only to `const` seems incomplete to me.

I think it's reasonable for `const` and `volatile` to go together, but I think 
the other specifiers are a different kind of setting because they have 
completely different rules for what they apply to.

  const int * pointer_to_const = nullptr;
  int const * also_pointer_to_const = nullptr;
  int * const const_pointer_to_mutable = nullptr;
  
  constexpr int * constexpr_pointer_to_mutable = nullptr;
  int constexpr * also_constexpr_pointer_to_mutable = nullptr;
  int * constexpr invalid = nullptr;

One of the main reasons people talk about putting `const` on the right is that 
then `const` always applies to the thing immediately to its left (except for 
cases that are weird regardless, like member functions). `constexpr` and other 
specifiers always apply to the top-level thing. Maybe I'm sheltered, but I 
don't see any users asking for the ability to format their code to consistently 
say `int constexpr x;`




Comment at: clang/docs/ClangFormatStyleOptions.rst:1378
 
+**ConstStyle** (``ConstAlignmentStyle``)
+  Different ways to arrange const.

steveire wrote:
> klimek wrote:
> > MyDeveloperDay wrote:
> > > aaron.ballman wrote:
> > > > MyDeveloperDay wrote:
> > > > > klimek wrote:
> > > > > > Personally, I'm somewhat against having 3 different aliases for the 
> > > > > > options. I'd chose one, even though it doesn't make everybody 
> > > > > > happy, and move on. I'm fine with East/West as long as the 
> > > > > > documentation makes it clear what it is.
> > > > > If I have to drop the other options, I think I'd want to go with 
> > > > > East/West const as I feel it has more momentum, just letting people 
> > > > > know before I change the code back (to my original patch ;-) )
> > > > > 
> > > > > https://www.youtube.com/watch?v=gRmI_gsNqcI
> > > > > 
> > > > > {F10954065}
> > > > > 
> > > > @klimek I requested that we do not go with East/West the options and 
> > > > I'm still pretty insistent on it. East/West is a kitschy way to phrase 
> > > > it that I think is somewhat US-centric (where we make a pretty big 
> > > > distinction between the east and west coasts). I do not want to have to 
> > > > mentally map left/right to the less-clear east/west in the config file. 
> > > > Would you be fine if we only had Left/Right instead of East/West? I 
> > > > would be fine with that option, but figured enough people like the cute 
> > > > East/West designation that we might as well support it.
> > > Just for a reference, I'm not from the US and I think east/west still 
> > > translates pretty well. I was happy to support the others. 
> > I'd be fine with only having left/right; my personal feeling is also that 
> > west-const / east-const has kinda become a term of art, though, so I really 
> > don't know which one is "right" :)
> > 
> > Generally, I think this is one of the cases where, given good docs, we're 
> > quickly spending more engineering hours discussing the right solution than 
> > it'll cost aggregated over all future users, under the assumption that 
> > people do not write new configs very often, and the few who will, will 
> > quickly remember.
> > 
> > I'd be fine with only having left/right; my personal feeling is also that 
> > west-const / east-const has kinda become a term of art, though, so I really 
> > don't know which one is "right" :)
> 
> This reminds me of the joke that Americans drive on the "Right" side of the 
> road, and English drive on the "Correct" side. Sort of gives a different 
> meaning to `ConstStyle : Right` and that the alternative is `Wrong` :). Maybe 
> that language ambiguity is why `East`/`West` caught on.
> 
> > people do not write new configs very often
> 
> Agreed. It seems a small number of strong views might influence this enough 
> to make `East`/`West` not be used. What a pity.
I'd also add that the original "joke" was `const west` vs `east const` -- the 
terms put the const where they would apply in the style. This seems to have 
gotten lost in the retelling and now the const is always on the right.

I'd favor `Left` and `Right`. `East` and `West` have "caught on" now, but it's 
caught on among the small group of C++ enthusiasts and still has a sort of 
ingroup joke feel to it. I instinctively know my preferred style is "east" 
because the phrase "east const" feels more natural to me because I've said it 
more. It took me a bit of repetition of thinking "East is right, I put the 
const on the right" to get there.

`East` and `West` also sets a precedent for future options to refer to 
positions as `North` and 

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: djasper.
aaron.ballman added a comment.

In D69764#2532413 , @steveire wrote:

> What can be done to move this change along?

Here is my thinking, which is largely unchanged from previous discussion: a 
code formatting tool should not modify code such that it changes meaning. This 
sort of behavior causes serious problems in practice, such as difficulty using 
the formatter in CI pipelines (because the user can't conform to what the 
formatter wants in some cases) or a lack of trust in the tool when it inserts 
subtle bugs that don't cause compile errors. I don't think that's acceptable 
for a production-quality tool that's expected to be run in a (semi-)automated 
fashion. I don't think the fact that clang-format already has such behavior is 
a blessing that we should add more such features. Every time we do that, we 
make a less usable tool for our users. So to me, the acceptance criteria is: 
the change should not break code, regardless of whether it's behind a flag or 
not. Flags alleviate some of the pain, certainly, but in clang-format, flags 
can be shared between multiple users due to the presence of a .clang-format 
file, and so "just change the configuration" isn't always a trivial bar for 
everyone to hop over.

I should note that I see the code breaking behavior between #include sort 
orders changing behavior and the qualifier position code breaking as being 
slightly different levels of severity. With include order, my code was brittle 
and the tool exposes that fact. With the qualifier change, my code doesn't 
necessarily have to be brittle to be broken.

All that said, clang-format is precisely where I'd *expect* this functionality 
to live, logically, so I agree with the strong desire to land the feature here. 
But given the option between fast and correct, I want both. If I can't have 
both, I want correct. So a shorter path forward to surfacing the feature 
appears to be within clang-tidy where we can use an AST to ensure we get 
correct behavior (at the expense of performance). There are a lot of longer 
paths forward, such as exploring why use of an AST is too slow for a code 
formatting tool and seeing if that can be improved (note, wins here are likely 
to wind up being wins for compilation speed with Clang overall, which would be 
very impactful!), but I can understand if the authors don't want to work on 
that effort. But I don't think "it's hard to be both fast and correct" is a 
valid reason to add known-problematic functionality to the code formatting tool 
with no known solution aside from "disable this functionality or find a 
different tool".


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D69764#2532952 , @sammccall wrote:

> In D69764#2532666 , @MyDeveloperDay 
> wrote:
>
>>> What can be done to move this change along?
>>
>> I feel there has to be a fundamental acceptance that it is ok for 
>> clang-format to alter code (something it already does with sorting of 
>> includes, namespace comments).
>>
>> There were fairly strong opinions that clang-format isn't the best tool to 
>> do this (which actually I don't agree with, I think it is, as long as those 
>> capabilities are off by default and there is an acceptance they won't be 
>> perfect especially in the presence of macros due to lack of AST)
>>
>> My only thought about building another tool would be if it was a drop in 
>> replacement for clang-format (tooling allows setting of a path), but it 
>> would need to inherit all of clang-format.
>> but to me, this just feels like extra grunt work just to work around why 
>> some community don't like it.
>
> Yeah, this seems like adding a flag with extra steps.
>
> clang-format's brand is:
>
> - fast
> - semantic no-op
> - applies a consistent, project-specific style
>
> I think putting it (permanently) behind a flag or alternate binary would cut 
> against #3. I don't like that.

It can naturally be behind a flag, with is: east, west, leave_it_be. Skittish 
people/teams can leave_it_be for a release or two.

> If it's buggy, this feature risks cutting against #2 (more than usual). So 
> code supporting this feature is more critical than it was previously, and 
> that might be a lot of heuristics.

How is the risk of this bugginess greater than the risk of any other 
transformation? (header sorting can expose invalid dependencies - and break the 
build).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D69764#2532666 , @MyDeveloperDay 
wrote:

>> What can be done to move this change along?
>
> I feel there has to be a fundamental acceptance that it is ok for 
> clang-format to alter code (something it already does with sorting of 
> includes, namespace comments).
>
> There were fairly strong opinions that clang-format isn't the best tool to do 
> this (which actually I don't agree with, I think it is, as long as those 
> capabilities are off by default and there is an acceptance they won't be 
> perfect especially in the presence of macros due to lack of AST)
>
> My only thought about building another tool would be if it was a drop in 
> replacement for clang-format (tooling allows setting of a path), but it would 
> need to inherit all of clang-format.
> but to me, this just feels like extra grunt work just to work around why some 
> community don't like it.

Yeah, this seems like adding a flag with extra steps.

clang-format's brand is:

- fast
- semantic no-op
- applies a consistent, project-specific style

I think putting it (permanently) behind a flag or alternate binary would cut 
against #3. I don't like that.

If it's buggy, this feature risks cutting against #2 (more than usual). So code 
supporting this feature is more critical than it was previously, and that might 
be a lot of heuristics.
So I'm wary, but also not really an active maintainer. As long as this concern 
has been considered, I'm not opposed!

> I guess a consensus is hard to come by, but I don't really know who owns the 
> decision around the future direction of clang-format.

In terms of practical maintainership, you're in a strong position to make this 
call. We've had a robust discussion, there are clear pros, cons, and some bits 
that aren't agreed.
@rsmith is CODE_OWNER for clang/... and so has a veto here, but doesn't sound 
inclined to use it :-)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D69764#2532666 , @MyDeveloperDay 
wrote:

>> What can be done to move this change along?
>
> I feel there has to be a fundamental acceptance that it is ok for 
> clang-format to alter code (something it already does with sorting of 
> includes, namespace comments).
>
> There were fairly strong opinions that clang-format isn't the best tool to do 
> this (which actually I don't agree with, I think it is, as long as those 
> capabilities are off by default and there is an acceptance they won't be 
> perfect especially in the presence of macros due to lack of AST)
>
> My only thought about building another tool would be if it was a drop in 
> replacement for clang-format (tooling allows setting of a path), but it would 
> need to inherit all of clang-format.
>
> but to me, this just feels like extra grunt work just to work around why some 
> community don't like it.
>
> I guess a consensus is hard to come by, but I don't really know who owns the 
> decision around the future direction of clang-format.

I wouldn't mind if it was implemented in clang-format.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.



> What can be done to move this change along?

I feel there has to be a fundamental acceptance that it is ok for clang-format 
to alter code (something it already does with sorting of includes, namespace 
comments).

There were fairly strong opinions that clang-format isn't the best tool to do 
this (which actually I don't agree with, I think it is, as long as those 
capabilities are off by default and there is an acceptance they won't be 
perfect especially in the presence of macros due to lack of AST)

My only thought about building another tool would be if it was a drop in 
replacement for clang-format (tooling allows setting of a path), but it would 
need to inherit all of clang-format.

but to me, this just feels like extra grunt work just to work around why some 
community don't like it.

I guess a consensus is hard to come by, but I don't really know who owns the 
decision around the future direction of clang-format.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

The idea has been floated to create a new different tool for changes like this 
(see eg D95168 ).

I don't think this should be done. These kinds of things should be in 
clang-format. One of the advantages of this and east/west const being in 
clang-format is that editors, CI systems and other tools have clang-format 
support. They would be unlikely to get support for a new tool. There are plenty 
of clang tools which exist but which don't get enough attention to get support 
in editors CI tools etc.

What can be done to move this change along?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D69764#2063798 , @MyDeveloperDay 
wrote:

> @steveire Thanks for the additional test cases, I'm reworking how I handle 
> the Macro case as I realized that I didn't actually even try to change the 
> case @rsmith came up with in the first place. I made a decision previous that 
> I couldn't handle any case without the *,& or && to determine that wasn't the 
> case
>
> Just ignoring upper case isn't a good idea either because of LONG,HRESULT and 
> LRESULT types.
>
> it may take me a couple of days, but I agree to have something akin to 
> FOREACH macros might be the way to go.


I think I've found more bugs in the current implementation, but I think you're 
working on a totally different implementation now, so there may not be so much 
value in reporting bugs with the current implementation, right?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

@steveire Thanks for the additional test cases, I'm reworking how I handle the 
Macro case as I realized that I didn't actually even try to change the case 
@rsmith came up with in the first place. I made a decision previous that I 
couldn't handle any case without the *,& or && to determine that wasn't the case

Just ignoring upper case isn't a good idea either because of LONG,HRESULT and 
LRESULT types.

it may take me a couple of days, but I agree to have something akin to FOREACH 
macros might be the way to go.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

Here's some more failing testcases.

  class Aa;
  class A;
  struct timespec;
  
  // Crash
  // #define UL unsigned long
  
  // Transformed, but with error reported:
  bool foo(Aa const &);
  
  // Not transformed (uppercase)
  template  bool bar(T const &);
  template  bool bat(TYPE const &);
  bool bing(A const &);
  
  // const inserted after struct. Does not compile
  void fot(struct timespec const t);
  
  // Not transformed
  template  void tov(typename Type::SubType const tu);
  
  // const inserted after typename. Does not compile
  template  void tor(typename Type::SubType const );
  template  void top(typename Type::SubType const *tu);
  
  // const inserted after `TYPE::` (because uppercase?)
  template  void top(typename TYPE::SubType const *tu);

This time I used

  BasedOnStyle: LLVM
  PointerAlignment: Left
  ConstPlacement: West

It seems to me that the heuristic "if the token is all uppercase assume it's a 
macro containing a * or &" is not the right heuristic. The mechanism of 
allowing the user to specify problematic macros in the config seems to make 
more sense.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Random thoughts from the peanut gallery:

- `clang-format` *is* the place I'd expect/want this feature, as a user. It's 
way more similar to `int *x` -> `int* x` than it is to e.g. typical clang-tidy 
rewrites. My only concern is whether we can give it the safety users expect.
- `clang-tidy` has a much higher barrier to entry: slow runtime, complex 
configuration, build-system integration, more false-positives in common 
scenarios.  e.g. Google has a pretty sophisticated CI system that integrates 
both: clang-format is typically blocking and clang-tidy is typically not. It 
would be less useful in clang-tidy.
- AIUI there are two identified sources of unsafety. They're real costs for 
sure, we should mitigate them as much as possible.
  - bugs: it seems reasonably likely these can be identified and fixed, based 
on what we've seen in this patch. I'd be more worried about maintenance if this 
was a drive-by contribution, but it's the opposite of that.
  - macros: there is some precedent for clang-format being bad or even unsafe 
in the presence of unknown macros. We could include a list of "don't touch 
qualifiers around" macro names in config. @klimek has a general-purpose 
mechanism nearly ready where you can provide actual macro definitions in 
config. It's also unclear if there's a common pattern where we'd see this.
  - typedefs don't introduce any such issues, right?
- one general mitigation is not including this in any default styles. We could 
go further and not support setting it in a style file (only as a flag) for 
clang-format 11, to shake out bugs.
- I think `const` is by far the most important qualifier to handle: volatile is 
rare, others cause less semantic confusion and are generally less 
controversial. IMO it's fine for this patch to only handle const as long as we 
know how the configuration could be expanded in future. Shipping is a feature.
- To bikeshed the configuration once more: what about `QualifierOrder: [Const, 
Type]`? This seems fairly self-explanatory, sidesteps "left" vs "west", and 
expands naturally to `[Static, Const, Type]` in future. It requires some 
nontrivial validation though.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D69764#2058590 , @rsmith wrote:

> In D69764#2058334 , @MyDeveloperDay 
> wrote:
>
> > @rsmith, Thank you for your comments, I don't agree, but thank you anyway.
> >
> > I will continue to feel there is some value here, My hope is that others 
> > will feel the same.
>
>
> To be clear: I do think a tool that will reorder specifiers to match a coding 
> standard has value. My concern is that it seems like scope creep for 
> clang-format in particular to do that, and that scope creep makes me 
> uncomfortable -- we're moving further away from the set of transformations 
> that clang-format can be very confident don't change the meaning of the 
> program, and we have seen problems with such scope creep in the past. But I'm 
> only uncomfortable, not opposed. Looking back through the review thread, I 
> don't think I'm raising anything that @sammccall didn't already bring up, and 
> it seems to me like you have consensus for doing this despite those concerns. 
> So be it. :)


I share the concern about this functionality causing a reformat to change the 
behavior of the program. I think a condition of accepting this feature into 
clang-format is that it not change the meaning of the user's code (except 
perhaps in very rare circumstances where there's consensus we should not care 
about that kind of input source code). If that means we can't have the feature 
in clang-format and need it in clang-tidy, so be it.

> I think that if we are reordering `const`, we should be reordering all 
> //decl-specifier//s -- I'd like to see `int static constexpr unsigned const 
> long inline` reordered to something like `static constexpr inline const 
> unsigned long int` too. Applying this only to `const` seems incomplete to me.

Agreed; I brought that up earlier in the review and still think it's valuable.




Comment at: clang/lib/Format/EastWestConstFixer.cpp:157-158
+  IsCVQualifierOrType(Tok->Next->Next)) {
+// The unsigned/signed case  `const unsigned int` -> `unsigned int
+// const`
+swapFirstThreeTokens(SourceMgr, Fixes, Tok, Tok->Next, Tok->Next->Next,

MyDeveloperDay wrote:
> MyDeveloperDay wrote:
> > aaron.ballman wrote:
> > > What about something like `const unsigned long long` or the 
> > > less-common-but-equally-valid `long const long unsigned`? Or multiple 
> > > qualifiers like `unsigned volatile long const long * restrict`
> > I would like to cover as many cases as possible, but a small part of me 
> > thinks that at least initially if we skip a case or two like this I'd be 
> > fine.
> > 
> > But I'll take a look and see what I think we could add easily in terms of 
> > multiple simple types in a row.
> @aaron.ballman  if you saw
> 
> `long const long unsigned` what would you expect it to become in both east 
> and west const cases? 
> 
> my assumption is:
> 
>  - east : `long long unsigned const`
>  - west: `const long long unsigned`
> 
> Or something else?
> 
> same for
> 
> `unsigned volatile long const long * restrict` I assume:
> 
>  - east : `unsigned volatile long long const * restrict`
>  - west: ` const unsigned volatile long long* restrict`
> 
> Its it would help if I understood the typical expectation in these situations?
> 
> 
I would assume the same thing you're assuming in these cases. Similar for:
```
long static constexpr unsigned long const int i = 12;

// East
static constexpr unsigned long long int const i = 12;
// West
static constexpr const unsigned long long int i = 12;
```
Uncertain how others feel, esp about the non-qualifier behavior, which I 
envision being: keep the non-qualifiers in whatever order they appear in the 
original source, shift the qualifiers left/right as needed (keeping the order 
in which they appear, if we decide to support all qualifiers instead of just 
`const` as part of this patch).



Comment at: clang/lib/Format/EastWestConstFixer.cpp:293
+  Tok->Next->Next->Next && Tok->Next->Next->Next->is(tok::kw_const)) {
+swapFourTokens(SourceMgr, Fixes, Tok, Tok->Next, Tok->Next, 
Tok->Next->Next->Next,
+ /*West=*/true);

curdeius wrote:
> MyDeveloperDay wrote:
> > rsmith wrote:
> > > There can be more than four type-specifiers / cv-qualifiers in a row. Eg: 
> > > `unsigned long long int volatile const` -> `const volatile unsigned long 
> > > long int`.
> > you have the volatile moving too? if you had the choice would it become:
> > 
> > - `const unsigned long long int volatile`
> > - `const volatile unsigned long long int`
> > - `volatile const unsigned long long int`
> > 
> > Any reason why? or is that personal taste? what would be the ideal?
> > 
> > 
> Given the size of this revision, it would be probably wiser not to touch 
> anything else than `const`.
> Any reason why? or is that personal taste? what would be the ideal?

It's a bit odd 

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/EastWestConstFixer.cpp:195
+FormatToken *Tok) {
+  // We only need to think about streams that begin with const.
+  if (!Tok->is(tok::kw_const)) {

curdeius wrote:
> MyDeveloperDay wrote:
> > curdeius wrote:
> > > Why? What about `unsigned const int`?
> > @curdeius would you help me understand your expectation here?
> > 
> >  - east: `unsigned int const`
> >  - west: `const unsigned int`
> > 
> > ?
> Yes, precisely this. And as for all other cases, I would only move `const`, 
> nothing else.
Ok that will now will work (but I'll add these specific unit tests to prove it)

`unsigned const int`

will see `unsigned const` and then swap it to be `const unsigned`

resulting in 

`const unsigned int`

similar will happen in the "east" const sense too (but again I'll add the tests)




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Marek Kurdej via Phabricator via cfe-commits]

curdeius marked an inline comment as done.
curdeius added inline comments.



Comment at: clang/lib/Format/EastWestConstFixer.cpp:195
+FormatToken *Tok) {
+  // We only need to think about streams that begin with const.
+  if (!Tok->is(tok::kw_const)) {

MyDeveloperDay wrote:
> curdeius wrote:
> > Why? What about `unsigned const int`?
> @curdeius would you help me understand your expectation here?
> 
>  - east: `unsigned int const`
>  - west: `const unsigned int`
> 
> ?
Yes, precisely this. And as for all other cases, I would only move `const`, 
nothing else.



Comment at: clang/lib/Format/EastWestConstFixer.cpp:293
+  Tok->Next->Next->Next && Tok->Next->Next->Next->is(tok::kw_const)) {
+swapFourTokens(SourceMgr, Fixes, Tok, Tok->Next, Tok->Next, 
Tok->Next->Next->Next,
+ /*West=*/true);

MyDeveloperDay wrote:
> rsmith wrote:
> > There can be more than four type-specifiers / cv-qualifiers in a row. Eg: 
> > `unsigned long long int volatile const` -> `const volatile unsigned long 
> > long int`.
> you have the volatile moving too? if you had the choice would it become:
> 
> - `const unsigned long long int volatile`
> - `const volatile unsigned long long int`
> - `volatile const unsigned long long int`
> 
> Any reason why? or is that personal taste? what would be the ideal?
> 
> 
Given the size of this revision, it would be probably wiser not to touch 
anything else than `const`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 266883.
MyDeveloperDay added a comment.

That's a rotate!

Remove the multiple swap functions for a single rotateTokens function (Thanks 
for the inspiration @curdeius)

extract the various combination of 2,3,4,5 qualifier types to a simple begin 
and end then pass those to the rotate.

Updating the diff prior to a potential config change


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/EastWestConstFixer.cpp
  clang/lib/Format/EastWestConstFixer.h
  clang/lib/Format/Format.cpp
  clang/tools/clang-format/ClangFormat.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13628,6 +13628,13 @@
   CHECK_PARSE("ContinuationIndentWidth: 11", ContinuationIndentWidth, 11u);
   CHECK_PARSE("CommentPragmas: '// abc$'", CommentPragmas, "// abc$");
 
+  Style.ConstPlacement = FormatStyle::CS_West;
+  CHECK_PARSE("ConstPlacement: Leave", ConstPlacement, FormatStyle::CS_Leave);
+  CHECK_PARSE("ConstPlacement: East", ConstPlacement, FormatStyle::CS_East);
+  CHECK_PARSE("ConstPlacement: West", ConstPlacement, FormatStyle::CS_West);
+  CHECK_PARSE("ConstPlacement: Right", ConstPlacement, FormatStyle::CS_East);
+  CHECK_PARSE("ConstPlacement: Left", ConstPlacement, FormatStyle::CS_West);
+
   Style.PointerAlignment = FormatStyle::PAS_Middle;
   CHECK_PARSE("PointerAlignment: Left", PointerAlignment,
   FormatStyle::PAS_Left);
@@ -16572,6 +16579,330 @@
"}",
Style);
 }
+
+TEST_F(FormatTest, EastWestConst) {
+  FormatStyle Style = getLLVMStyle();
+
+  // keep the const style unaltered
+  verifyFormat("const int a;", Style);
+  verifyFormat("const int *a;", Style);
+  verifyFormat("const int ", Style);
+  verifyFormat("const int &", Style);
+  verifyFormat("int const b;", Style);
+  verifyFormat("int const *b;", Style);
+  verifyFormat("int const ", Style);
+  verifyFormat("int const &", Style);
+  verifyFormat("int const *b const;", Style);
+  verifyFormat("int *const c;", Style);
+
+  verifyFormat("const Foo a;", Style);
+  verifyFormat("const Foo *a;", Style);
+  verifyFormat("const Foo ", Style);
+  verifyFormat("const Foo &", Style);
+  verifyFormat("Foo const b;", Style);
+  verifyFormat("Foo const *b;", Style);
+  verifyFormat("Foo const ", Style);
+  verifyFormat("Foo const &", Style);
+  verifyFormat("Foo const *b const;", Style);
+
+  verifyFormat("LLVM_NODISCARD const int ();", Style);
+  verifyFormat("LLVM_NODISCARD int const ();", Style);
+
+  verifyFormat("volatile const int *restrict;", Style);
+  verifyFormat("const volatile int *restrict;", Style);
+  verifyFormat("const int volatile *restrict;", Style);
+}
+
+TEST_F(FormatTest, EastConst) {
+  FormatStyle Style = getLLVMStyle();
+  Style.ConstPlacement = FormatStyle::CS_East;
+
+  verifyFormat("int const a;", Style);
+  verifyFormat("int const *a;", Style);
+  verifyFormat("int const ", Style);
+  verifyFormat("int const &", Style);
+  verifyFormat("int const b;", Style);
+  verifyFormat("int const *b;", Style);
+  verifyFormat("int const ", Style);
+  verifyFormat("int const &", Style);
+  verifyFormat("int const *b const;", Style);
+  verifyFormat("int *const c;", Style);
+
+  verifyFormat("Foo const a;", Style);
+  verifyFormat("Foo const *a;", Style);
+  verifyFormat("Foo const ", Style);
+  verifyFormat("Foo const &", Style);
+  verifyFormat("Foo const b;", Style);
+  verifyFormat("Foo const *b;", Style);
+  verifyFormat("Foo const ", Style);
+  verifyFormat("Foo const &", Style);
+  verifyFormat("Foo const *b const;", Style);
+  verifyFormat("Foo *const b;", Style);
+  verifyFormat("Foo const *const b;", Style);
+  verifyFormat("auto const v = get_value();", Style);
+  verifyFormat("long long const ", Style);
+  verifyFormat("unsigned char const *a;", Style);
+  verifyFormat("int main(int const argc, char const *const *const argv)",
+   Style);
+
+  verifyFormat("LLVM_NODISCARD int const ();", Style);
+  verifyFormat("SourceRange getSourceRange() const override LLVM_READONLY",
+   Style);
+  verifyFormat("void foo() const override;", Style);
+  verifyFormat("void foo() const override LLVM_READONLY;", Style);
+  verifyFormat("void foo() const final;", Style);
+  verifyFormat("void foo() const final LLVM_READONLY;", Style);
+  verifyFormat("void foo() const LLVM_READONLY;", Style);
+
+  verifyFormat(
+  "template  explicit Action(Action const );",
+  Style);
+  verifyFormat(
+  "template  explicit Action(Action const );",
+  "template  explicit Action(const Action& action);",
+  Style);
+  verifyFormat(
+  "template 

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 3 inline comments as done.
MyDeveloperDay added a comment.

In D69764#2058801 , @curdeius wrote:

> One last thought, how about making the config a bit more future-proof and 
> instead of `ConstPlacement` accept what was discussed some time ago:
>
>   QualifierStyle:
>  -   const: Right
>
>
> and restrict it to `const` for the moment?
>  Maybe renaming to `QualifierPlacement` or something more appropriate.


I'm already feeling there needs to be more fine grained control here in the 
config... (even if that is a list of MACRO types to ignore) or 
IgnoreCapitializedIdentifiers, or WarnButDontFix or as we talked about before 
handling more than just the placement of const, I feel you are correct I'll end 
up polluting the toplevel config namespace unless I switch to some form of 
structure in the YAML like we do with BraceWrapping.




Comment at: clang/lib/Format/EastWestConstFixer.cpp:157-158
+  IsCVQualifierOrType(Tok->Next->Next)) {
+// The unsigned/signed case  `const unsigned int` -> `unsigned int
+// const`
+swapFirstThreeTokens(SourceMgr, Fixes, Tok, Tok->Next, Tok->Next->Next,

MyDeveloperDay wrote:
> aaron.ballman wrote:
> > What about something like `const unsigned long long` or the 
> > less-common-but-equally-valid `long const long unsigned`? Or multiple 
> > qualifiers like `unsigned volatile long const long * restrict`
> I would like to cover as many cases as possible, but a small part of me 
> thinks that at least initially if we skip a case or two like this I'd be fine.
> 
> But I'll take a look and see what I think we could add easily in terms of 
> multiple simple types in a row.
@aaron.ballman  if you saw

`long const long unsigned` what would you expect it to become in both east and 
west const cases? 

my assumption is:

 - east : `long long unsigned const`
 - west: `const long long unsigned`

Or something else?

same for

`unsigned volatile long const long * restrict` I assume:

 - east : `unsigned volatile long long const * restrict`
 - west: ` const unsigned volatile long long* restrict`

Its it would help if I understood the typical expectation in these situations?





Comment at: clang/lib/Format/EastWestConstFixer.cpp:195
+FormatToken *Tok) {
+  // We only need to think about streams that begin with const.
+  if (!Tok->is(tok::kw_const)) {

curdeius wrote:
> Why? What about `unsigned const int`?
@curdeius would you help me understand your expectation here?

 - east: `unsigned int const`
 - west: `const unsigned int`

?



Comment at: clang/lib/Format/EastWestConstFixer.cpp:293
+  Tok->Next->Next->Next && Tok->Next->Next->Next->is(tok::kw_const)) {
+swapFourTokens(SourceMgr, Fixes, Tok, Tok->Next, Tok->Next, 
Tok->Next->Next->Next,
+ /*West=*/true);

rsmith wrote:
> There can be more than four type-specifiers / cv-qualifiers in a row. Eg: 
> `unsigned long long int volatile const` -> `const volatile unsigned long long 
> int`.
you have the volatile moving too? if you had the choice would it become:

- `const unsigned long long int volatile`
- `const volatile unsigned long long int`
- `volatile const unsigned long long int`

Any reason why? or is that personal taste? what would be the ideal?




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/EastWestConstFixer.cpp:130
+
+static void swapFourTokens(const SourceManager ,
+ tooling::Replacements ,

curdeius wrote:
> These functions seem a bit ugly... and very specific. And they both look like 
> rotate left/right. Couldn't it be a single function taking a 
> range/span/collection of FormatTokens?
This is something I'd also started to feel the same, now I'm starting to handle 
longer sets of qualifier, I can replace all these swap functions for one single 
rotate.

```
lang=c++
static void rotateTokens(const SourceManager ,
 tooling::Replacements , const FormatToken *First,
 const FormatToken *Last, bool Left) {
  auto *End = Last;
  auto *Begin = First;
  if (!Left) {
End = Last->Next;
Begin = First->Next;
  }

  std::string NewText;
  // If we are rotating to the left we move the Last token to the front.
  if (Left) {
NewText += Last->TokenText;
NewText += " ";
  }

  // Then move through the other tokens.
  auto *Tok = Begin;
  while (Tok != End) {
if (!NewText.empty())
  NewText += " ";

NewText += Tok->TokenText;
Tok = Tok->Next;
  }

  // If we are rotating to the right we move the first token to the back.
  if (!Left) {
NewText += " ";
NewText += First->TokenText;
  }

  auto Range = CharSourceRange::getCharRange(First->getStartOfNonWhitespace(),
 Last->Tok.getEndLoc());
  auto Err = Fixes.add(tooling::Replacement(SourceMgr, Range, NewText));
  if (Err) {
llvm::errs() << "Error while rearranging const : "
 << llvm::toString(std::move(Err)) << "\n";
  }
}
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 266799.
MyDeveloperDay added a comment.

Minor change for the simpler review comments before refactoring some of the 
more involved ones


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/EastWestConstFixer.cpp
  clang/lib/Format/EastWestConstFixer.h
  clang/lib/Format/Format.cpp
  clang/tools/clang-format/ClangFormat.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13628,6 +13628,13 @@
   CHECK_PARSE("ContinuationIndentWidth: 11", ContinuationIndentWidth, 11u);
   CHECK_PARSE("CommentPragmas: '// abc$'", CommentPragmas, "// abc$");
 
+  Style.ConstPlacement = FormatStyle::CS_West;
+  CHECK_PARSE("ConstPlacement: Leave", ConstPlacement, FormatStyle::CS_Leave);
+  CHECK_PARSE("ConstPlacement: East", ConstPlacement, FormatStyle::CS_East);
+  CHECK_PARSE("ConstPlacement: West", ConstPlacement, FormatStyle::CS_West);
+  CHECK_PARSE("ConstPlacement: Right", ConstPlacement, FormatStyle::CS_East);
+  CHECK_PARSE("ConstPlacement: Left", ConstPlacement, FormatStyle::CS_West);
+
   Style.PointerAlignment = FormatStyle::PAS_Middle;
   CHECK_PARSE("PointerAlignment: Left", PointerAlignment,
   FormatStyle::PAS_Left);
@@ -16572,6 +16579,326 @@
"}",
Style);
 }
+
+TEST_F(FormatTest, EastWestConst) {
+  FormatStyle Style = getLLVMStyle();
+
+  // keep the const style unaltered
+  verifyFormat("const int a;", Style);
+  verifyFormat("const int *a;", Style);
+  verifyFormat("const int ", Style);
+  verifyFormat("const int &", Style);
+  verifyFormat("int const b;", Style);
+  verifyFormat("int const *b;", Style);
+  verifyFormat("int const ", Style);
+  verifyFormat("int const &", Style);
+  verifyFormat("int const *b const;", Style);
+  verifyFormat("int *const c;", Style);
+
+  verifyFormat("const Foo a;", Style);
+  verifyFormat("const Foo *a;", Style);
+  verifyFormat("const Foo ", Style);
+  verifyFormat("const Foo &", Style);
+  verifyFormat("Foo const b;", Style);
+  verifyFormat("Foo const *b;", Style);
+  verifyFormat("Foo const ", Style);
+  verifyFormat("Foo const &", Style);
+  verifyFormat("Foo const *b const;", Style);
+
+  verifyFormat("LLVM_NODISCARD const int ();", Style);
+  verifyFormat("LLVM_NODISCARD int const ();", Style);
+
+  verifyFormat("volatile const int *restrict;", Style);
+  verifyFormat("const volatile int *restrict;", Style);
+  verifyFormat("const int volatile *restrict;", Style);
+}
+
+TEST_F(FormatTest, EastConst) {
+  FormatStyle Style = getLLVMStyle();
+  Style.ConstPlacement = FormatStyle::CS_East;
+
+  
+  verifyFormat("int const a;", Style);
+  verifyFormat("int const *a;", Style);
+  verifyFormat("int const ", Style);
+  verifyFormat("int const &", Style);
+  verifyFormat("int const b;", Style);
+  verifyFormat("int const *b;", Style);
+  verifyFormat("int const ", Style);
+  verifyFormat("int const &", Style);
+  verifyFormat("int const *b const;", Style);
+  verifyFormat("int *const c;", Style);
+
+  verifyFormat("Foo const a;", Style);
+  verifyFormat("Foo const *a;", Style);
+  verifyFormat("Foo const ", Style);
+  verifyFormat("Foo const &", Style);
+  verifyFormat("Foo const b;", Style);
+  verifyFormat("Foo const *b;", Style);
+  verifyFormat("Foo const ", Style);
+  verifyFormat("Foo const &", Style);
+  verifyFormat("Foo const *b const;", Style);
+  verifyFormat("Foo *const b;", Style);
+  verifyFormat("Foo const *const b;", Style);
+  verifyFormat("auto const v = get_value();", Style);
+  verifyFormat("long long const ", Style);
+  verifyFormat("unsigned char const *a;", Style);
+  verifyFormat("int main(int const argc, char const *const *const argv)",
+   Style);
+
+  verifyFormat("LLVM_NODISCARD int const ();", Style);
+  verifyFormat("SourceRange getSourceRange() const override LLVM_READONLY",
+   Style);
+  verifyFormat("void foo() const override;", Style);
+  verifyFormat("void foo() const override LLVM_READONLY;", Style);
+  verifyFormat("void foo() const final;", Style);
+  verifyFormat("void foo() const final LLVM_READONLY;", Style);
+  verifyFormat("void foo() const LLVM_READONLY;", Style);
+
+  verifyFormat(
+  "template  explicit Action(Action const );",
+  Style);
+  verifyFormat(
+  "template  explicit Action(Action const );",
+  "template  explicit Action(const Action& action);",
+  Style);
+  verifyFormat(
+  "template  explicit Action(Action const );",
+  "template \nexplicit Action(const Action& action);",
+  Style);
+
+  verifyFormat("int const a;", "const int a;", Style);
+  verifyFormat("int const 

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

One last thought, how about making the config a bit more future-proof and 
instead of `ConstPlacement` accept what was discussed some time ago:

  QualifierStyle:
 -   const: Right

and restrict it to `const` for the moment?
Maybe renaming to `QualifierPlacement` or something more appropriate.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:359
 
+- Option ``ConstPlacement`` has been added auto-arrange the positioning of 
const
+  in variable and parameter declarations to be ``Right/East`` const or 
``Left/West`` 

It sounds strange as if you wanted to write "[in order] to auto-arrange".



Comment at: clang/lib/Format/EastWestConstFixer.cpp:83
+  // Change `const int` to be `int const`.
+  std::string NewType;
+  NewType += Second->TokenText;

Nit: `NewType` may be misleading when reading. Why not `NewText` as above?



Comment at: clang/lib/Format/EastWestConstFixer.cpp:130
+
+static void swapFourTokens(const SourceManager ,
+ tooling::Replacements ,

These functions seem a bit ugly... and very specific. And they both look like 
rotate left/right. Couldn't it be a single function taking a 
range/span/collection of FormatTokens?



Comment at: clang/lib/Format/EastWestConstFixer.cpp:136
+ const FormatToken *Fourth, 
+ bool West) {
+  // e.g. Change `const unsigned long long` to be `unsigned long long const`.

Nit: West doesn't seem appropriate as a name at the level of this function as 
you rather don't move elements west/east but left/right. Of course, that 
applies only if you share my view that it's sort of rotate algorithm.



Comment at: clang/lib/Format/EastWestConstFixer.cpp:171
+return false;
+  return (Tok->isSimpleTypeSpecifier() ||
+  Tok->isOneOf(tok::kw_volatile, tok::kw_auto));

Parentheses seem to be unnecessary.



Comment at: clang/lib/Format/EastWestConstFixer.cpp:171
+return false;
+  return (Tok->isSimpleTypeSpecifier() ||
+  Tok->isOneOf(tok::kw_volatile, tok::kw_auto));

curdeius wrote:
> Parentheses seem to be unnecessary.
Stupid question, are both const and restrict handled here?



Comment at: clang/lib/Format/EastWestConstFixer.cpp:175
+
+// If a token is an identifier and its upper case it could
+// be a macro and hence we need to be able to ignore it

Typo: its -> it's.
Missing comma before "it could".



Comment at: clang/lib/Format/EastWestConstFixer.cpp:176
+// If a token is an identifier and its upper case it could
+// be a macro and hence we need to be able to ignore it
+static bool isPossibleMacro(const FormatToken *Tok)

Missing dot at the end.



Comment at: clang/lib/Format/EastWestConstFixer.cpp:195
+FormatToken *Tok) {
+  // We only need to think about streams that begin with const.
+  if (!Tok->is(tok::kw_const)) {

Why? What about `unsigned const int`?



Comment at: clang/lib/Format/EastWestConstFixer.cpp:208
+
+  if (isCVQualifierOrType(Tok->Next) && isCVQualifierOrType(Tok->Next->Next) 
&& isCVQualifierOrType(Tok->Next->Next->Next)) {
+swapFourTokens(SourceMgr, Fixes, Tok, Tok->Next, Tok->Next->Next, 
Tok->Next->Next->Next,

I think that this code asks for a cleanup. And if we needed to look for five 
tokens...?



Comment at: clang/lib/Format/EastWestConstFixer.cpp:213
+  }
+  if (isCVQualifierOrType(Tok->Next) && Tok->Next->Next &&
+  isCVQualifierOrType(Tok->Next->Next)) {

Shouldn't it be `else if`?



Comment at: clang/lib/Format/EastWestConstFixer.cpp:223
+ Tok->Next->is(tok::kw_auto)) {
+// The basic case  `const int` -> `int const`
+swapTwoTokens(SourceMgr, Fixes, Tok, Tok->Next);

Nits: double space, missing ending dot.



Comment at: clang/lib/Format/EastWestConstFixer.cpp:225
+swapTwoTokens(SourceMgr, Fixes, Tok, Tok->Next);
+
+  } else if (Tok->startsSequence(tok::kw_const, tok::identifier,

Nit: unnecessary blank line.



Comment at: clang/lib/Format/EastWestConstFixer.cpp:228
+ TT_TemplateOpener)) {
+// Read from to the end of the TemplateOpener to
+// TemplateCloser const ArrayRef a; const ArrayRef 

"from to the end"?



Comment at: clang/lib/Format/EastWestConstFixer.cpp:244
+  removeToken(SourceMgr, Fixes, Tok);
+  //return EndTemplate->Next;
+  return Tok;

Forgotten remnants?



Comment at: clang/lib/Format/EastWestConstFixer.cpp:383
+while (Tok && Tok != Last) {
+  if (!Tok->Next) {
+break;

It's a matter of taste, but this condition could be moved into the while 
condition above.



Comment at: clang/lib/Format/EastWestConstFixer.cpp:387
+  if (Tok->is(tok::comment)) {
+Tok = Tok->Next;
+continue;

You 

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D69764#2058334 , @MyDeveloperDay 
wrote:

> @rsmith, Thank you for your comments, I don't agree, but thank you anyway.
>
> I will continue to feel there is some value here, My hope is that others will 
> feel the same.


To be clear: I do think a tool that will reorder specifiers to match a coding 
standard has value. My concern is that it seems like scope creep for 
clang-format in particular to do that, and that scope creep makes me 
uncomfortable -- we're moving further away from the set of transformations that 
clang-format can be very confident don't change the meaning of the program, and 
we have seen problems with such scope creep in the past. But I'm only 
uncomfortable, not opposed. Looking back through the review thread, I don't 
think I'm raising anything that @sammccall didn't already bring up, and it 
seems to me like you have consensus for doing this despite those concerns. So 
be it. :)

I think that if we are reordering `const`, we should be reordering all 
//decl-specifier//s -- I'd like to see `int static constexpr unsigned const 
long inline` reordered to something like `static constexpr inline const 
unsigned long int` too. Applying this only to `const` seems incomplete to me.




Comment at: clang/lib/Format/EastWestConstFixer.cpp:199
+  }
+  // Don't concern youself if nothing follows const.
+  if (!Tok->Next) {

(Typo: youself -> yourself)



Comment at: clang/lib/Format/EastWestConstFixer.cpp:291
+  if (isCVQualifierOrType(Tok) && isCVQualifierOrType(Tok->Next) && 
isCVQualifierOrType(Tok->Next->Next) &&
+// `unsigned longl long const` -> `const unsigned long long`.
+  Tok->Next->Next->Next && Tok->Next->Next->Next->is(tok::kw_const)) {

(Typo: longl -> long)



Comment at: clang/lib/Format/EastWestConstFixer.cpp:293
+  Tok->Next->Next->Next && Tok->Next->Next->Next->is(tok::kw_const)) {
+swapFourTokens(SourceMgr, Fixes, Tok, Tok->Next, Tok->Next, 
Tok->Next->Next->Next,
+ /*West=*/true);

There can be more than four type-specifiers / cv-qualifiers in a row. Eg: 
`unsigned long long int volatile const` -> `const volatile unsigned long long 
int`.



Comment at: clang/lib/Format/EastWestConstFixer.h:1
+//===--- EastWwestConstFixer.h --*- C++ 
-*-===//
+//

(Typo in file name.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

@rsmith, Thank you for your comments, I don't agree, but thank you anyway.

I will continue to feel there is some value here, My hope is that others will 
feel the same.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 266647.
MyDeveloperDay added a comment.

Begin to cover the cases raised as potential code-breaking changes by ignoring 
potential MACRO usage
add support for more complex `const unsigned long long` cases   (I believe I 
can simplify this to a more general pattern)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/EastWestConstFixer.cpp
  clang/lib/Format/EastWestConstFixer.h
  clang/lib/Format/Format.cpp
  clang/tools/clang-format/ClangFormat.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13628,6 +13628,13 @@
   CHECK_PARSE("ContinuationIndentWidth: 11", ContinuationIndentWidth, 11u);
   CHECK_PARSE("CommentPragmas: '// abc$'", CommentPragmas, "// abc$");
 
+  Style.ConstPlacement = FormatStyle::CS_West;
+  CHECK_PARSE("ConstPlacement: Leave", ConstPlacement, FormatStyle::CS_Leave);
+  CHECK_PARSE("ConstPlacement: East", ConstPlacement, FormatStyle::CS_East);
+  CHECK_PARSE("ConstPlacement: West", ConstPlacement, FormatStyle::CS_West);
+  CHECK_PARSE("ConstPlacement: Right", ConstPlacement, FormatStyle::CS_East);
+  CHECK_PARSE("ConstPlacement: Left", ConstPlacement, FormatStyle::CS_West);
+
   Style.PointerAlignment = FormatStyle::PAS_Middle;
   CHECK_PARSE("PointerAlignment: Left", PointerAlignment,
   FormatStyle::PAS_Left);
@@ -16572,6 +16579,326 @@
"}",
Style);
 }
+
+TEST_F(FormatTest, EastWestConst) {
+  FormatStyle Style = getLLVMStyle();
+
+  // keep the const style unaltered
+  verifyFormat("const int a;", Style);
+  verifyFormat("const int *a;", Style);
+  verifyFormat("const int ", Style);
+  verifyFormat("const int &", Style);
+  verifyFormat("int const b;", Style);
+  verifyFormat("int const *b;", Style);
+  verifyFormat("int const ", Style);
+  verifyFormat("int const &", Style);
+  verifyFormat("int const *b const;", Style);
+  verifyFormat("int *const c;", Style);
+
+  verifyFormat("const Foo a;", Style);
+  verifyFormat("const Foo *a;", Style);
+  verifyFormat("const Foo ", Style);
+  verifyFormat("const Foo &", Style);
+  verifyFormat("Foo const b;", Style);
+  verifyFormat("Foo const *b;", Style);
+  verifyFormat("Foo const ", Style);
+  verifyFormat("Foo const &", Style);
+  verifyFormat("Foo const *b const;", Style);
+
+  verifyFormat("LLVM_NODISCARD const int ();", Style);
+  verifyFormat("LLVM_NODISCARD int const ();", Style);
+
+  verifyFormat("volatile const int *restrict;", Style);
+  verifyFormat("const volatile int *restrict;", Style);
+  verifyFormat("const int volatile *restrict;", Style);
+}
+
+TEST_F(FormatTest, EastConst) {
+  FormatStyle Style = getLLVMStyle();
+  Style.ConstPlacement = FormatStyle::CS_East;
+
+  
+  verifyFormat("int const a;", Style);
+  verifyFormat("int const *a;", Style);
+  verifyFormat("int const ", Style);
+  verifyFormat("int const &", Style);
+  verifyFormat("int const b;", Style);
+  verifyFormat("int const *b;", Style);
+  verifyFormat("int const ", Style);
+  verifyFormat("int const &", Style);
+  verifyFormat("int const *b const;", Style);
+  verifyFormat("int *const c;", Style);
+
+  verifyFormat("Foo const a;", Style);
+  verifyFormat("Foo const *a;", Style);
+  verifyFormat("Foo const ", Style);
+  verifyFormat("Foo const &", Style);
+  verifyFormat("Foo const b;", Style);
+  verifyFormat("Foo const *b;", Style);
+  verifyFormat("Foo const ", Style);
+  verifyFormat("Foo const &", Style);
+  verifyFormat("Foo const *b const;", Style);
+  verifyFormat("Foo *const b;", Style);
+  verifyFormat("Foo const *const b;", Style);
+  verifyFormat("auto const v = get_value();", Style);
+  verifyFormat("long long const ", Style);
+  verifyFormat("unsigned char const *a;", Style);
+  verifyFormat("int main(int const argc, char const *const *const argv)",
+   Style);
+
+  verifyFormat("LLVM_NODISCARD int const ();", Style);
+  verifyFormat("SourceRange getSourceRange() const override LLVM_READONLY",
+   Style);
+  verifyFormat("void foo() const override;", Style);
+  verifyFormat("void foo() const override LLVM_READONLY;", Style);
+  verifyFormat("void foo() const final;", Style);
+  verifyFormat("void foo() const final LLVM_READONLY;", Style);
+  verifyFormat("void foo() const LLVM_READONLY;", Style);
+
+  verifyFormat(
+  "template  explicit Action(Action const );",
+  Style);
+  verifyFormat(
+  "template  explicit Action(Action const );",
+  "template  explicit Action(const Action& action);",
+  Style);
+  verifyFormat(
+  "template  explicit Action(Action const );",
+  "template \nexplicit 

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D69764#2057945 , @aaron.ballman 
wrote:

> In D69764#2056104 , @rsmith wrote:
>
> > I also don't think this is something that a user would *want* in 
> > `clang-format`: changing the location of `const`s is something that would 
> > likely be done rarely (probably only once) when migrating to a different 
> > coding style, rather than being done on the fly after each edit to a source 
> > file.
>
>
> I'm not certain I agree with this. For instance, you can run clang-format as 
> a precommit step or as part of your CI pipeline to bark at users when they 
> get formatting incorrect while working on a team project. (We do this 
> internally with some of our internal projects.)


You can do the same with clang-tidy checks. If you're running clang-format but 
not clang-tidy as part of your CI, it would likely be worth your while to add 
clang-tidy to your set of CI checks regardless of what we do here. But if you 
want this only as a CI check, and not for reformatting code as you edit it, 
then for me that is a fairly strong signal that this does not belong in 
clang-format.

>> Fundamentally, I don't think this transformation is simply reformatting, and 
>> I don't think it can be done sufficiently-correctly with only a 
>> largely-context-free, heuristic-driven C++ parser. As such, I think this 
>> belongs in `clang-tidy`, not in `clang-format`.
> 
> I think clang-tidy has the machinery needed to do this properly, but I think 
> clang-format is logically where I would go to look for the functionality 
> (certainly before thinking of clang-tidy) because this is more related to 
> formatting than not. That said, if we cannot make it work reliably within 
> clang-format, I'd rather see it in clang-tidy than nowhere.

Right now we have a relatively clear line between the tools. clang-format does 
not parse or really understand the code, and just heuristically puts the 
whitespace and line breaks in the right place, in a way that is ~always 
correct. clang-tidy understands the meaning of the program and can suggest 
changes that are likely correct (but should typically be double-checked by a 
person). I think this kind of change is in the latter category.

I totally agree that it's reasonable to think of this as a reformatting change, 
just as I think it's reasonable to think of (say) reordering the data members 
of a class to the start as a reformatting change, or to think of moving an 
inline function definition out of the class definition as a reformatting 
change, or parenthesizing certain operators as a reformatting change -- and all 
of those could also reasonably be required by some house coding style. But I 
don't think clang-format is the right tool to perform those operations.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D69764#2056104 , @rsmith wrote:

> I'm uncomfortable about `clang-format` performing this transformation at all. 
> Generally, clang-format only makes changes that are guaranteed to preserve 
> the meaning of the source program, and does not make changes that are only 
> heuristically likely to be semantics-preserving. But this transformation is 
> not behavior-preserving in a variety of cases, such as:
>
>   #define INTPTR int *
>   const INTPTR a;
>   // INTPTR const a; means something else
>


That's an excellent point. I agree that the formatting cannot change the 
meaning of the code that's being formatted -- that would be bad.

> I also don't think this is something that a user would *want* in 
> `clang-format`: changing the location of `const`s is something that would 
> likely be done rarely (probably only once) when migrating to a different 
> coding style, rather than being done on the fly after each edit to a source 
> file.

I'm not certain I agree with this. For instance, you can run clang-format as a 
precommit step or as part of your CI pipeline to bark at users when they get 
formatting incorrect while working on a team project. (We do this internally 
with some of our internal projects.)

> Fundamentally, I don't think this transformation is simply reformatting, and 
> I don't think it can be done sufficiently-correctly with only a 
> largely-context-free, heuristic-driven C++ parser. As such, I think this 
> belongs in `clang-tidy`, not in `clang-format`.

I think clang-tidy has the machinery needed to do this properly, but I think 
clang-format is logically where I would go to look for the functionality 
(certainly before thinking of clang-tidy) because this is more related to 
formatting than not. That said, if we cannot make it work reliably within 
clang-format, I'd rather see it in clang-tidy than nowhere.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

@MyDeveloperDay +1 from the trenches - I am in that same position and it took a 
lot of work to get the organization to _align_ on a consistent style, then 
enforce that.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 266460.
MyDeveloperDay added a comment.

Fix crash whilst rechecking polly

  ../polly/lib/Analysis/ScopBuilder.cpp:74:8: warning: code should be 
clang-format
  ted [-Wclang-format-violations]
  static int const MaxDimensionsInAccessRange = 9;
 ^
  ../polly/lib/Analysis/ScopInfo.cpp:114:1: warning: code should be 
clang-formatte
  d [-Wclang-format-violations]
  int const polly::MaxDisjunctsInDomain = 20;
  ^
  ../polly/lib/Analysis/ScopInfo.cpp:119:8: warning: code should be 
clang-formatte
  d [-Wclang-format-violations]
  static int const MaxDisjunctsInContext = 4;
 ^
  ../polly/lib/Analysis/ScopInfo.cpp:2686:3: warning: code should be 
clang-formatt
  ed [-Wclang-format-violations]
auto const  = F->getParent()->getDataLayout();
^
  ../polly/lib/Analysis/ScopInfo.cpp:2822:3: warning: code should be 
clang-formatt
  ed [-Wclang-format-violations]
auto const  = F.getParent()->getDataLayout();
^
  ../../build_ninja/bin/clang-format -n ../polly/lib/CodeGen/*.cpp
  ../../build_ninja/bin/clang-format -n ../polly/lib/Plugin/*.cpp
  ../../build_ninja/bin/clang-format -n ../polly/lib/Transform/*.cpp
  ../polly/lib/Transform/Simplify.cpp:39:8: warning: code should be 
clang-formatte
  d [-Wclang-format-violations]
  static int const SimplifyMaxDisjuncts = 4;
 ^
  ../../build_ninja/bin/clang-format -n ../polly/lib/Support/*.cpp
  ../polly/lib/Support/SCEVAffinator.cpp:35:8: warning: code should be 
clang-forma
  tted [-Wclang-format-violations]
  static int const MaxDisjunctionsInPwAff = 100;
 ^
  ../polly/lib/Support/SCEVAffinator.cpp:39:8: warning: code should be 
clang-forma
  tted [-Wclang-format-violations]
  static unsigned const MaxSmallBitWidth = 7;
 ^


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/EastWestConstFixer.cpp
  clang/lib/Format/EastWestConstFixer.h
  clang/lib/Format/Format.cpp
  clang/tools/clang-format/ClangFormat.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13628,6 +13628,13 @@
   CHECK_PARSE("ContinuationIndentWidth: 11", ContinuationIndentWidth, 11u);
   CHECK_PARSE("CommentPragmas: '// abc$'", CommentPragmas, "// abc$");
 
+  Style.ConstPlacement = FormatStyle::CS_West;
+  CHECK_PARSE("ConstPlacement: Leave", ConstPlacement, FormatStyle::CS_Leave);
+  CHECK_PARSE("ConstPlacement: East", ConstPlacement, FormatStyle::CS_East);
+  CHECK_PARSE("ConstPlacement: West", ConstPlacement, FormatStyle::CS_West);
+  CHECK_PARSE("ConstPlacement: Right", ConstPlacement, FormatStyle::CS_East);
+  CHECK_PARSE("ConstPlacement: Left", ConstPlacement, FormatStyle::CS_West);
+
   Style.PointerAlignment = FormatStyle::PAS_Middle;
   CHECK_PARSE("PointerAlignment: Left", PointerAlignment,
   FormatStyle::PAS_Left);
@@ -16572,6 +16579,301 @@
"}",
Style);
 }
+
+TEST_F(FormatTest, EastWestConst) {
+  FormatStyle Style = getLLVMStyle();
+
+  // keep the const style unaltered
+  verifyFormat("const int a;", Style);
+  verifyFormat("const int *a;", Style);
+  verifyFormat("const int ", Style);
+  verifyFormat("const int &", Style);
+  verifyFormat("int const b;", Style);
+  verifyFormat("int const *b;", Style);
+  verifyFormat("int const ", Style);
+  verifyFormat("int const &", Style);
+  verifyFormat("int const *b const;", Style);
+  verifyFormat("int *const c;", Style);
+
+  verifyFormat("const Foo a;", Style);
+  verifyFormat("const Foo *a;", Style);
+  verifyFormat("const Foo ", Style);
+  verifyFormat("const Foo &", Style);
+  verifyFormat("Foo const b;", Style);
+  verifyFormat("Foo const *b;", Style);
+  verifyFormat("Foo const ", Style);
+  verifyFormat("Foo const &", Style);
+  verifyFormat("Foo const *b const;", Style);
+
+  verifyFormat("LLVM_NODISCARD const int ();", Style);
+  verifyFormat("LLVM_NODISCARD int const ();", Style);
+
+  verifyFormat("volatile const int *restrict;", Style);
+  verifyFormat("const volatile int *restrict;", Style);
+  verifyFormat("const int volatile *restrict;", Style);
+}
+
+TEST_F(FormatTest, EastConst) {
+  FormatStyle Style = getLLVMStyle();
+  Style.ConstPlacement = FormatStyle::CS_East;
+
+  verifyFormat("int const a;", Style);
+  verifyFormat("int const *a;", Style);
+  verifyFormat("int const ", Style);
+  verifyFormat("int const &", Style);
+  verifyFormat("int const b;", Style);
+  verifyFormat("int const *b;", Style);
+  verifyFormat("int const ", Style);
+  verifyFormat("int const &", Style);
+  verifyFormat("int const *b const;", Style);
+  verifyFormat("int *const c;", Style);
+
+  verifyFormat("Foo 

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added a comment.

@rsmith, firstly let me thank you for taking the time to review this, I didn't 
realize i'd invoke such a reaction that the big guns would start showing up.. 
fundamentally I agree with you, but let me defend my thesis to help explain my 
reasons why I still feel this has value.

clang-format is in my view no longer just a code formatter used for 
transforming whitespace, This changed when we added the sorting of headers and 
the adding of namespace comments, but fundamentally clang-format has always 
worked on what is basically the tooling::Replacements interface, extending this 
to alter code is a likely natural progression.

Whilst I understand your point about clang-tidy, alas I agree with @steveire 
its not a viable solution in my view and I don't think it would catch the case 
you suggest either. Its also somewhat slower and needs so much more 
information, In very large million line projects its akin to a nightly build 
and not a sub second sanity check. (In my view clang-tidy is  not the right 
tool for this job).

I do understand they will be failure scenarios, (and maybe there is something 
we can do about those), but...

Clang-format has always been able to break your code especially in extraneous 
circumstances where people use macros (the `__LINE__` example here made me 
smile!)
https://stackoverflow.com/questions/37927553/can-clang-format-break-my-code

And a recent well publicized issue highlighted that even if we change something 
that seems semantically the same, it may not be! But that isn't a reason to not 
use clang-format in my view, its just a reason to be more careful.
https://travisdowns.github.io/blog/2019/11/19/toupper.html

But for me this change is NOT about those corner cases because clang-format is 
not bug free, it does make mistakes, as such it WILL break code, and even if it 
doesn't break it semantically, it will from time to time it makes it look like 
it threw up on your editor (or its drunk 
https://twitter.com/Manu343726/status/1124686897819934720)  , but we have a 
workaround for those cases, `//clang-format off` for anything we can't fix and 
http://bugs.llvm.org for those we can.

You are correct in that this change is most useful during the conversion of a 
project, and as such its value might initially seem limited, this is why its an 
`opt in` option, `Leave/East/West` with the Default always being to "Leave as 
is". However anyone bringing in a new clang-format option or clang-format 
completely always needs to be careful when reviewing the changes it suggests, 
there will be corner cases (especially around macros) as there are with other 
clang-format options which can break the code.

My expectation is that its most useful usage is with the command line argument 
--const-placement=west/east being added to clang-format being run in dry-run 
mode as part of premerge type checking and used to reject code review which 
fails that..  I highlighted this once before in this trail run (in polly), 
where code has slipped in as east const into LLVM and missed during review.

  clang-format -n --const-placement=west *.cpp
  
  ScopBuilder.cpp:74:9: warning: code should be clang-formatted 
[-Wclang-format-violations]
  static int const MaxDimensionsInAccessRange = 9;
  ^
  ScopInfo.cpp:113:2: warning: code should be clang-formatted 
[-Wclang-format-violations]
  int const polly::MaxDisjunctsInDomain = 20;
  
   ^

But for someone like me managing a multi million line C++ code base with 
developers in the 4 corners of the globe with a constant turnover of new staff, 
this is the ultimate value.. I no longer have to persuade some new senior 
engineer who thinks they have been hired for their coding prowess who insists 
their bracketing and coding style is the most beautiful in the world when the 
rest of us know its butt ugly.!  Quite simply they cannot get their code 
committed unless it conforms to our style guide, not because I say so, but 
because the faceless tool of clang-format says "No!", there is value right 
there in reducing the arguments and stress alone.

LLVM's own pre-merge checking is reducing our need to keep tell people to 
clang-format in code review, and this change is about building that ability to 
reject code before it gets committed, to reduce the code review burden.

And Finally this change is about trying to banish the inane conversations about 
what "const" is "best const" in the same way as we have somewhat done with 
white-space and tabs.  I just don't think we should waste our time talking 
about it, just clang-format it and "you do you", this is what I want out of 
this because I'm fed of up seeing brilliant minds 
(http://slashslash.info/2018/02/a-foolish-consistency/)  argue about it when a 
99.9% solution is a couple of 100 line patch.

This is the defense of my work and why I and I believe others think there is 
value here.


CHANGES SINCE LAST 

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D69764#2056104 , @rsmith wrote:

> I'm uncomfortable about `clang-format` performing this transformation at all. 
> Generally, clang-format only makes changes that are guaranteed to preserve 
> the meaning of the source program, and does not make changes that are only 
> heuristically likely to be semantics-preserving. But this transformation is 
> not behavior-preserving in a variety of cases, such as:
>
>   #define INTPTR int *
>   const INTPTR a;
>   // INTPTR const a; means something else
>


At least in my case, our codebase doesn't have code like that. I wonder how 
common it is.

> I also don't think this is something that a user would *want* in 
> `clang-format`: changing the location of `const`s is something that would 
> likely be done rarely (probably only once) when migrating to a different 
> coding style, rather than being done on the fly after each edit to a source 
> file.

I don't think this is true.

All of the arguments in favor of `clang-format` existing apply here.

- Developers get it wrong. They put the `{` or the `*` in the "wrong" place 
according to the style guide, and they put the `const` in the "wrong" place 
according to the style guide.
- `clang-format` is much faster than `clang-tidy` and it is reasonable to have 
text editors run it on every "Save" and on all files in a repo on every git 
commit etc.
- The above means that the cost of developers getting it wrong is minimized or 
eliminated
- The above means that developers don't have to pay attention to `*` placement 
or `const` placement while writing code, but can (in theory at least) focus on 
what they're trying to convey.
- It seems that more tools understand `clang-format` than `clang-tidy` (eg text 
editors with support for it)

> Fundamentally, I don't think this transformation is simply reformatting, and 
> I don't think it can be done sufficiently-correctly with only a 
> largely-context-free, heuristic-driven C++ parser.

I agree that this is a less simple transformation than existing `clang-format` 
functionality.

I can't evaluate whether your macro example (or other examples you could come 
up with) mean that it can't be done sufficiently-correctly, but I doubt I would 
come to the same conclusion. I would probably base that on whether I could find 
real-world code which it breaks, and how common the breaking-patterns (the 
macro in your example) actually are in other code.

> As such, I think this belongs in `clang-tidy`, not in `clang-format`.

Given the speed difference and the developer convenience, I think this would be 
very unfortunate. I've already tried to write this conversion as a `clang-tidy` 
check. However, my implementation has far more bugs than this `clang-format` 
implementation, and it does not handle as many cases. You can see that many 
`clang-tidy` checks exclude types of code such as "all templates" from 
transformation to dampen the complexity of the check implementation.

Besides, a clang-tidy implementation would run into the same problem with your 
macro example, wouldn't it? Often the solution to that kind of problem in 
`clang-tidy` checks is to simply not transform code in macros. Would an option 
in clang-format to not transform around macro code be an more acceptable 
solution?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I'm uncomfortable about `clang-format` performing this transformation at all. 
Generally, clang-format only makes changes that are guaranteed to preserve the 
meaning of the source program, and does not make changes that are only 
heuristically likely to be semantics-preserving. But this transformation is not 
behavior-preserving in a variety of cases, such as:

  #define INTPTR int *
  const INTPTR a;
  // INTPTR const a; means something else

I also don't think this is something that a user would *want* in 
`clang-format`: changing the location of `const`s is something that would 
likely be done rarely (probably only once) when migrating to a different coding 
style, rather than being done on the fly after each edit to a source file.

Fundamentally, I don't think this transformation is simply reformatting, and I 
don't think it can be done sufficiently-correctly with only a 
largely-context-free, heuristic-driven C++ parser. As such, I think this 
belongs in `clang-tidy`, not in `clang-format`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 266360.
MyDeveloperDay added a comment.

rebase with master


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/EastWestConstFixer.cpp
  clang/lib/Format/EastWestConstFixer.h
  clang/lib/Format/Format.cpp
  clang/tools/clang-format/ClangFormat.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13628,6 +13628,13 @@
   CHECK_PARSE("ContinuationIndentWidth: 11", ContinuationIndentWidth, 11u);
   CHECK_PARSE("CommentPragmas: '// abc$'", CommentPragmas, "// abc$");
 
+  Style.ConstPlacement = FormatStyle::CS_West;
+  CHECK_PARSE("ConstPlacement: Leave", ConstPlacement, FormatStyle::CS_Leave);
+  CHECK_PARSE("ConstPlacement: East", ConstPlacement, FormatStyle::CS_East);
+  CHECK_PARSE("ConstPlacement: West", ConstPlacement, FormatStyle::CS_West);
+  CHECK_PARSE("ConstPlacement: Right", ConstPlacement, FormatStyle::CS_East);
+  CHECK_PARSE("ConstPlacement: Left", ConstPlacement, FormatStyle::CS_West);
+
   Style.PointerAlignment = FormatStyle::PAS_Middle;
   CHECK_PARSE("PointerAlignment: Left", PointerAlignment,
   FormatStyle::PAS_Left);
@@ -16572,6 +16579,301 @@
"}",
Style);
 }
+
+TEST_F(FormatTest, EastWestConst) {
+  FormatStyle Style = getLLVMStyle();
+
+  // keep the const style unaltered
+  verifyFormat("const int a;", Style);
+  verifyFormat("const int *a;", Style);
+  verifyFormat("const int ", Style);
+  verifyFormat("const int &", Style);
+  verifyFormat("int const b;", Style);
+  verifyFormat("int const *b;", Style);
+  verifyFormat("int const ", Style);
+  verifyFormat("int const &", Style);
+  verifyFormat("int const *b const;", Style);
+  verifyFormat("int *const c;", Style);
+
+  verifyFormat("const Foo a;", Style);
+  verifyFormat("const Foo *a;", Style);
+  verifyFormat("const Foo ", Style);
+  verifyFormat("const Foo &", Style);
+  verifyFormat("Foo const b;", Style);
+  verifyFormat("Foo const *b;", Style);
+  verifyFormat("Foo const ", Style);
+  verifyFormat("Foo const &", Style);
+  verifyFormat("Foo const *b const;", Style);
+
+  verifyFormat("LLVM_NODISCARD const int ();", Style);
+  verifyFormat("LLVM_NODISCARD int const ();", Style);
+
+  verifyFormat("volatile const int *restrict;", Style);
+  verifyFormat("const volatile int *restrict;", Style);
+  verifyFormat("const int volatile *restrict;", Style);
+}
+
+TEST_F(FormatTest, EastConst) {
+  FormatStyle Style = getLLVMStyle();
+  Style.ConstPlacement = FormatStyle::CS_East;
+
+  verifyFormat("int const a;", Style);
+  verifyFormat("int const *a;", Style);
+  verifyFormat("int const ", Style);
+  verifyFormat("int const &", Style);
+  verifyFormat("int const b;", Style);
+  verifyFormat("int const *b;", Style);
+  verifyFormat("int const ", Style);
+  verifyFormat("int const &", Style);
+  verifyFormat("int const *b const;", Style);
+  verifyFormat("int *const c;", Style);
+
+  verifyFormat("Foo const a;", Style);
+  verifyFormat("Foo const *a;", Style);
+  verifyFormat("Foo const ", Style);
+  verifyFormat("Foo const &", Style);
+  verifyFormat("Foo const b;", Style);
+  verifyFormat("Foo const *b;", Style);
+  verifyFormat("Foo const ", Style);
+  verifyFormat("Foo const &", Style);
+  verifyFormat("Foo const *b const;", Style);
+  verifyFormat("Foo *const b;", Style);
+  verifyFormat("Foo const *const b;", Style);
+  verifyFormat("auto const v = get_value();", Style);
+  verifyFormat("long long const ", Style);
+  verifyFormat("unsigned char const *a;", Style);
+  verifyFormat("int main(int const argc, char const *const *const argv)",
+   Style);
+
+  verifyFormat("LLVM_NODISCARD int const ();", Style);
+  verifyFormat("SourceRange getSourceRange() const override LLVM_READONLY",
+   Style);
+  verifyFormat("void foo() const override;", Style);
+  verifyFormat("void foo() const override LLVM_READONLY;", Style);
+  verifyFormat("void foo() const final;", Style);
+  verifyFormat("void foo() const final LLVM_READONLY;", Style);
+  verifyFormat("void foo() const LLVM_READONLY;", Style);
+
+  verifyFormat(
+  "template  explicit Action(Action const );",
+  Style);
+  verifyFormat(
+  "template  explicit Action(Action const );",
+  "template  explicit Action(const Action& action);",
+  Style);
+  verifyFormat(
+  "template  explicit Action(Action const );",
+  "template \nexplicit Action(const Action& action);",
+  Style);
+
+  verifyFormat("int const a;", "const int a;", Style);
+  verifyFormat("int const *a;", "const int *a;", Style);
+  verifyFormat("int const ", "const int ", Style);
+  

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 8 inline comments as done.
MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/EastWestConstFixer.cpp:139
+  return (Tok->isSimpleTypeSpecifier() ||
+  Tok->isOneOf(tok::kw_volatile, tok::kw_auto));
+}

aaron.ballman wrote:
> Do you need to look for `restrict` here as well as `volatile`?
I think restrict only occurs the other side of the * am I right?



Comment at: clang/lib/Format/EastWestConstFixer.cpp:157-158
+  IsCVQualifierOrType(Tok->Next->Next)) {
+// The unsigned/signed case  `const unsigned int` -> `unsigned int
+// const`
+swapFirstThreeTokens(SourceMgr, Fixes, Tok, Tok->Next, Tok->Next->Next,

aaron.ballman wrote:
> What about something like `const unsigned long long` or the 
> less-common-but-equally-valid `long const long unsigned`? Or multiple 
> qualifiers like `unsigned volatile long const long * restrict`
I would like to cover as many cases as possible, but a small part of me thinks 
that at least initially if we skip a case or two like this I'd be fine.

But I'll take a look and see what I think we could add easily in terms of 
multiple simple types in a row.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 266358.
MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added a comment.

Fix issue when preprocessor #if/#else is present
Rename the config file name to `ConstPlacement`
change the command line option to be `--const-placement`
Add Left/Right into the documentation


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/EastWestConstFixer.cpp
  clang/lib/Format/EastWestConstFixer.h
  clang/lib/Format/Format.cpp
  clang/tools/clang-format/ClangFormat.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13628,6 +13628,13 @@
   CHECK_PARSE("ContinuationIndentWidth: 11", ContinuationIndentWidth, 11u);
   CHECK_PARSE("CommentPragmas: '// abc$'", CommentPragmas, "// abc$");
 
+  Style.ConstPlacement = FormatStyle::CS_West;
+  CHECK_PARSE("ConstPlacement: Leave", ConstPlacement, FormatStyle::CS_Leave);
+  CHECK_PARSE("ConstPlacement: East", ConstPlacement, FormatStyle::CS_East);
+  CHECK_PARSE("ConstPlacement: West", ConstPlacement, FormatStyle::CS_West);
+  CHECK_PARSE("ConstPlacement: Right", ConstPlacement, FormatStyle::CS_East);
+  CHECK_PARSE("ConstPlacement: Left", ConstPlacement, FormatStyle::CS_West);
+
   Style.PointerAlignment = FormatStyle::PAS_Middle;
   CHECK_PARSE("PointerAlignment: Left", PointerAlignment,
   FormatStyle::PAS_Left);
@@ -16567,6 +16574,301 @@
"}",
Style);
 }
+
+TEST_F(FormatTest, EastWestConst) {
+  FormatStyle Style = getLLVMStyle();
+
+  // keep the const style unaltered
+  verifyFormat("const int a;", Style);
+  verifyFormat("const int *a;", Style);
+  verifyFormat("const int ", Style);
+  verifyFormat("const int &", Style);
+  verifyFormat("int const b;", Style);
+  verifyFormat("int const *b;", Style);
+  verifyFormat("int const ", Style);
+  verifyFormat("int const &", Style);
+  verifyFormat("int const *b const;", Style);
+  verifyFormat("int *const c;", Style);
+
+  verifyFormat("const Foo a;", Style);
+  verifyFormat("const Foo *a;", Style);
+  verifyFormat("const Foo ", Style);
+  verifyFormat("const Foo &", Style);
+  verifyFormat("Foo const b;", Style);
+  verifyFormat("Foo const *b;", Style);
+  verifyFormat("Foo const ", Style);
+  verifyFormat("Foo const &", Style);
+  verifyFormat("Foo const *b const;", Style);
+
+  verifyFormat("LLVM_NODISCARD const int ();", Style);
+  verifyFormat("LLVM_NODISCARD int const ();", Style);
+
+  verifyFormat("volatile const int *restrict;", Style);
+  verifyFormat("const volatile int *restrict;", Style);
+  verifyFormat("const int volatile *restrict;", Style);
+}
+
+TEST_F(FormatTest, EastConst) {
+  FormatStyle Style = getLLVMStyle();
+  Style.ConstPlacement = FormatStyle::CS_East;
+
+  verifyFormat("int const a;", Style);
+  verifyFormat("int const *a;", Style);
+  verifyFormat("int const ", Style);
+  verifyFormat("int const &", Style);
+  verifyFormat("int const b;", Style);
+  verifyFormat("int const *b;", Style);
+  verifyFormat("int const ", Style);
+  verifyFormat("int const &", Style);
+  verifyFormat("int const *b const;", Style);
+  verifyFormat("int *const c;", Style);
+
+  verifyFormat("Foo const a;", Style);
+  verifyFormat("Foo const *a;", Style);
+  verifyFormat("Foo const ", Style);
+  verifyFormat("Foo const &", Style);
+  verifyFormat("Foo const b;", Style);
+  verifyFormat("Foo const *b;", Style);
+  verifyFormat("Foo const ", Style);
+  verifyFormat("Foo const &", Style);
+  verifyFormat("Foo const *b const;", Style);
+  verifyFormat("Foo *const b;", Style);
+  verifyFormat("Foo const *const b;", Style);
+  verifyFormat("auto const v = get_value();", Style);
+  verifyFormat("long long const ", Style);
+  verifyFormat("unsigned char const *a;", Style);
+  verifyFormat("int main(int const argc, char const *const *const argv)",
+   Style);
+
+  verifyFormat("LLVM_NODISCARD int const ();", Style);
+  verifyFormat("SourceRange getSourceRange() const override LLVM_READONLY",
+   Style);
+  verifyFormat("void foo() const override;", Style);
+  verifyFormat("void foo() const override LLVM_READONLY;", Style);
+  verifyFormat("void foo() const final;", Style);
+  verifyFormat("void foo() const final LLVM_READONLY;", Style);
+  verifyFormat("void foo() const LLVM_READONLY;", Style);
+
+  verifyFormat(
+  "template  explicit Action(Action const );",
+  Style);
+  verifyFormat(
+  "template  explicit Action(Action const );",
+  "template  explicit Action(const Action& action);",
+  Style);
+  verifyFormat(
+  "template  explicit Action(Action const );",
+  "template 

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Stephen Kelly via Phabricator via cfe-commits]

steveire added inline comments.



Comment at: clang/lib/Format/EastWestConstFixer.cpp:22
+
+#define DEBUG_TYPE "using-declarations-sorter"
+

Should this be removed?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D69764#2055716 , @MyDeveloperDay 
wrote:

> I really do appreciate the reviews and the comments especially regarding 
> east/west/left/right/wrong ;-), I know there won't be 100% agreement, but I 
> think most people who even considered cloning the LLVM repo know what we mean 
> by East/West. As such I'm going to leave the internal code that way for now. 
> (and the name of the TokenAnalyzer Files)


I think that's totally reasonable. People working on dev tools are more likely 
to be aware of newer terminology anyway.

> However, I have added support for Left/Right and East/West in the config, but 
> I'm going to refrain from adding Before/After otherwise it just gets silly 
> having too many options.

Sounds good to me!

> Whilst I've been developing this I've tried both ways, to be honest, I 
> instinctively know what is meant by East Const because its the style I don't 
> use myself (don't shoot me for that comment). But when talking in terms of 
> Left/Right I feel I have to think about what it means quite hard. Especially 
> as Right feels Wrong to me too!

Would it help if we named the option `ConstPlacement` (or something along those 
lines) instead of `ConstStyle` as a reminder that this is about the placement 
of the qualifier relative to the base type? Or we could keep `ConstStyle` and 
go with `OnRight`|`OnLeft` (in addition to `East`|`West`) if that reads more 
clearly.

> Let me reiterate my goal. For me the goal was to make east/west const 
> conversations disappear in the same way that tab and whitespace conversations 
> have disappeared (mostly) because I think those conversations are a waste of 
> good conference time.

I think it's a great goal and I'm really looking forward to having this option 
in clang-format -- thank you for working on this feature!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

>   if I put any declarations inside the preprocess clauses they actually don't 
> get converted.

Sorry, I'm not certain what this means. Does it mean that if you have

  #if 0
  Foo> P;
  #else
  Foo> P;
  #endif

that neither of them get converted?

Can you point me to a git branch I can use to try this out? The last patch I 
tried to download from phab didn't apply cleanly. If you have a git branch I 
can rebase it with more confidence.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I really do appreciate the reviews and the comments especially regarding 
east/west/left/right/wrong ;-), I know there won't be 100% agreement, but I 
think most people who even consider clone the LLVM repo know what we mean by 
East/West. as such I'm going to leave the internal code that way for now. (and 
the name of the TokenAnalyzer Files)

However, I have added support for Left/Right and East/West in the config, but 
I'm going to refrain from adding Before/After otherwise it just gets silly 
having too many options.

Whilst I've been developing this I've tried both ways, to be honest I 
instinctively know what is meant by East Const because its the style I don't 
use personally (don't shoot me for that comment). But when taking in terms of 
Left/Right I feel I have to think about what it means quite hard. Especially as 
Right feels Wrong to me!

Let me reiterate my goal. For me the goal was to make east/west const 
conversations disappear in the same way that tab and whitespace conversations 
have disappeared (mostly) because I think those conversations are a waste of 
good conference time.

The answer should just be "use clang-format", and for me, this is part of my 
own feeling that we should "clang-format all the things". I feel the best 
compromise is to offer both, (I'll likely update the documentation to not have 
so much of a bias)

If we can agree to that, then I'll be happy in a year or so to do a poll of the 
.clang-format files in github.com and then change the internal code to match 
the majority, to me that would be the greatest measure of which should be the 
primary option.

Apart from that, I've still some work to do here, this case from @steveire is 
really stumping me. Every preprocessor branch causes a different iteration over 
the same original code and as such this is causing multiple replacements to be 
added as each pass reanalyses the original code and doesn't regenerate the code 
in between (super odd)

  verifyFormat("Foo> P;\n#if 0\n#else\n#endif", "Foo 
const> P;\n#if 0\n#else\n#endif", Style);

I tried to only perform the replacement on the first pass, but alas that means 
if I put any declarations inside the preprocess clauses they actually don't get 
converted. (I'm not sure if anyone has seen this before), but its likely the 
fact that I'm using clang-format to create replacements.

In the background I've been testing this on LLVM itself (which isn't easy 
because the code isn't all clang-formatted itself, another pet peeve of mine), 
apart from the #if issue (which hits lib/Analyzer/CFG.cpp and a few other 
files) it seems to work pretty well (although I've not done a detailed 
walkthrough to see if it's missing much)

I would like to land this at some point, but cannot whilst I still have the 
#if/#else issue

Thanks for the help and the feedback, just wanted to give everyone a progress 
report.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/Format.cpp:2547
 
+  if (Style.isCpp() || Style.Language == FormatStyle::LK_ObjC) {
+if (Style.ConstStyle != FormatStyle::CS_Leave)

aaron.ballman wrote:
> MyDeveloperDay wrote:
> > aaron.ballman wrote:
> > > This prevents us from using this in C code despite C having qualifiers 
> > > that can go to the left or right of the base type but still allows you to 
> > > use if from Objective-C. That seems incorrect.
> > clang-format's isCpp() covers C and C++ (and ObjectiveC and ObjectiveC++)
> > 
> > but you did highlight that I don't need the extra LK_ObjC check
> > clang-format's isCpp() covers C and C++ (and ObjectiveC and ObjectiveC++)
> 
> Wow, that's a really poorly named function then! Thank you for the 
> clarification.
I've been trying to persuade people ;-) {D80079}


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

@MyDeveloperDay Thanks for the update. I pinged you on slack about this, but I 
guess you're not using it at the moment. I asked if you have a git branch 
somewhere with this change. Downloading patches from phab is such a pain I have 
no idea why we use it.

If you can link me to a branch somehow, I can re-test this.

Regarding

  #if 0
  #else
  #endif

blocks causing multiple re-parses, presumably this is because clang-format 
formats code in the "other" preprocessor branch? At least I think it reformats 
comments in that case. Maybe the problem can be worked around with that in mind.




Comment at: clang/docs/ClangFormatStyleOptions.rst:1378
 
+**ConstStyle** (``ConstAlignmentStyle``)
+  Different ways to arrange const.

klimek wrote:
> MyDeveloperDay wrote:
> > aaron.ballman wrote:
> > > MyDeveloperDay wrote:
> > > > klimek wrote:
> > > > > Personally, I'm somewhat against having 3 different aliases for the 
> > > > > options. I'd chose one, even though it doesn't make everybody happy, 
> > > > > and move on. I'm fine with East/West as long as the documentation 
> > > > > makes it clear what it is.
> > > > If I have to drop the other options, I think I'd want to go with 
> > > > East/West const as I feel it has more momentum, just letting people 
> > > > know before I change the code back (to my original patch ;-) )
> > > > 
> > > > https://www.youtube.com/watch?v=gRmI_gsNqcI
> > > > 
> > > > {F10954065}
> > > > 
> > > @klimek I requested that we do not go with East/West the options and I'm 
> > > still pretty insistent on it. East/West is a kitschy way to phrase it 
> > > that I think is somewhat US-centric (where we make a pretty big 
> > > distinction between the east and west coasts). I do not want to have to 
> > > mentally map left/right to the less-clear east/west in the config file. 
> > > Would you be fine if we only had Left/Right instead of East/West? I would 
> > > be fine with that option, but figured enough people like the cute 
> > > East/West designation that we might as well support it.
> > Just for a reference, I'm not from the US and I think east/west still 
> > translates pretty well. I was happy to support the others. 
> I'd be fine with only having left/right; my personal feeling is also that 
> west-const / east-const has kinda become a term of art, though, so I really 
> don't know which one is "right" :)
> 
> Generally, I think this is one of the cases where, given good docs, we're 
> quickly spending more engineering hours discussing the right solution than 
> it'll cost aggregated over all future users, under the assumption that people 
> do not write new configs very often, and the few who will, will quickly 
> remember.
> 
> I'd be fine with only having left/right; my personal feeling is also that 
> west-const / east-const has kinda become a term of art, though, so I really 
> don't know which one is "right" :)

This reminds me of the joke that Americans drive on the "Right" side of the 
road, and English drive on the "Correct" side. Sort of gives a different 
meaning to `ConstStyle : Right` and that the alternative is `Wrong` :). Maybe 
that language ambiguity is why `East`/`West` caught on.

> people do not write new configs very often

Agreed. It seems a small number of strong views might influence this enough to 
make `East`/`West` not be used. What a pity.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Format/EastWestConstFixer.cpp:139
+  return (Tok->isSimpleTypeSpecifier() ||
+  Tok->isOneOf(tok::kw_volatile, tok::kw_auto));
+}

Do you need to look for `restrict` here as well as `volatile`?



Comment at: clang/lib/Format/EastWestConstFixer.cpp:150
+  }
+  // don't concern youself if nothing follows const
+  if (!Tok->Next) {

Comments should start with a capital letter and end with punctuation per the 
coding standard (same applies to other comments in the patch).



Comment at: clang/lib/Format/EastWestConstFixer.cpp:157-158
+  IsCVQualifierOrType(Tok->Next->Next)) {
+// The unsigned/signed case  `const unsigned int` -> `unsigned int
+// const`
+swapFirstThreeTokens(SourceMgr, Fixes, Tok, Tok->Next, Tok->Next->Next,

What about something like `const unsigned long long` or the 
less-common-but-equally-valid `long const long unsigned`? Or multiple 
qualifiers like `unsigned volatile long const long * restrict`



Comment at: clang/lib/Format/Format.cpp:2547
 
+  if (Style.isCpp() || Style.Language == FormatStyle::LK_ObjC) {
+if (Style.ConstStyle != FormatStyle::CS_Leave)

MyDeveloperDay wrote:
> aaron.ballman wrote:
> > This prevents us from using this in C code despite C having qualifiers that 
> > can go to the left or right of the base type but still allows you to use if 
> > from Objective-C. That seems incorrect.
> clang-format's isCpp() covers C and C++ (and ObjectiveC and ObjectiveC++)
> 
> but you did highlight that I don't need the extra LK_ObjC check
> clang-format's isCpp() covers C and C++ (and ObjectiveC and ObjectiveC++)

Wow, that's a really poorly named function then! Thank you for the 
clarification.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Gašper Ažman via cfe-commits]

I prefer east/west, but I think there's a terminology that is
uncontroversial (left/right) and one that is controversial. It's also clear
to me that it's better to have only one set of terms (aliases are only fine
for backwards compatibility). Left/Right won, I think.

On Tue, May 26, 2020 at 1:55 PM Sam McCall via Phabricator <
revi...@reviews.llvm.org> wrote:

> sammccall added a comment.
>
> Thanks @MyDeveloperDay for hammering on on these bugs and @steveire for
> finding them! There's still obviously some risk here but as long as this is
> opt-in for a major release or two (i.e. not turned on in built-in styles,
> any remaining bugs should get flushed out.
>
> Regarding option naming, I did think East/West (only) was the way to go
> but have reluctantly changed my mind after rereading the discussion. My
> conclusions were:
>
> - C++ people who have read discussions on this from 2018 on are likely
> familiar with "east const" terminology
> - there are people who care about style who aren't familiar with it and
> wouldn't be happy to have to learn/adopt it (this was the main surprise for
> me)
> - 5 years from now, this naming may have spread to ~everyone, may remain
> partially-adopted, or possibly even die out as a fad
> - for people familiar with the terminology: "ConstStyle: East" is clearer
> than "ConstStyle: Right" (less ambiguous)
> - for people unfamiliar with the terminology, the opposite is certainly
> true
> - asymmetry 1: it's probably harder to work out east=right than to to work
> out that "ConstStyle: right" means const is written on the right of the type
> - asymmetry 2: the new terminology is objectively better: terse,
> memorable, doesn't collide with other terms. Some dislike it, which is true
> of any distinctive name (I hate "namespace", for instance).
> - there's clearly a cultural tension between reading like a meme-infested
> subreddit and like an IBM technical manual :-)
>
> It's a tough call, but I'd propose a compromise: make Left/Right canonical
> as it's more accessible (don't have to learn new things) and in case
> East/West dies out.
> But to have East/West as aliases: to let the community decide over time,
> and because I don't think we should be in the business of hindering
> adoption of better names.
>
> ("reluctantly" changed my mind because I do think east/west are better
> names. But meeting users where they are is important too - otherwise we'd
> just hardcode the One True Style :-))
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D69764/new/
>
> https://reviews.llvm.org/D69764
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks @MyDeveloperDay for hammering on on these bugs and @steveire for finding 
them! There's still obviously some risk here but as long as this is opt-in for 
a major release or two (i.e. not turned on in built-in styles, any remaining 
bugs should get flushed out.

Regarding option naming, I did think East/West (only) was the way to go but 
have reluctantly changed my mind after rereading the discussion. My conclusions 
were:

- C++ people who have read discussions on this from 2018 on are likely familiar 
with "east const" terminology
- there are people who care about style who aren't familiar with it and 
wouldn't be happy to have to learn/adopt it (this was the main surprise for me)
- 5 years from now, this naming may have spread to ~everyone, may remain 
partially-adopted, or possibly even die out as a fad
- for people familiar with the terminology: "ConstStyle: East" is clearer than 
"ConstStyle: Right" (less ambiguous)
- for people unfamiliar with the terminology, the opposite is certainly true
- asymmetry 1: it's probably harder to work out east=right than to to work out 
that "ConstStyle: right" means const is written on the right of the type
- asymmetry 2: the new terminology is objectively better: terse, memorable, 
doesn't collide with other terms. Some dislike it, which is true of any 
distinctive name (I hate "namespace", for instance).
- there's clearly a cultural tension between reading like a meme-infested 
subreddit and like an IBM technical manual :-)

It's a tough call, but I'd propose a compromise: make Left/Right canonical as 
it's more accessible (don't have to learn new things) and in case East/West 
dies out.
But to have East/West as aliases: to let the community decide over time, and 
because I don't think we should be in the business of hindering adoption of 
better names.

("reluctantly" changed my mind because I do think east/west are better names. 
But meeting users where they are is important too - otherwise we'd just 
hardcode the One True Style :-))


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits