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

26 views
Skip to first unread message

MyDeveloperDay via Phabricator

unread,
May 24, 2020, 6:27:54 PM5/24/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
MyDeveloperDay planned changes to this revision.
MyDeveloperDay added a comment.

@steveire I'm still working on this I have just one issue from your lit that is failing (see below), but I wanted to capture the other changes in the review.

const Foo<Foo<int>>* p = const_cast<const Foo<Foo<int>>*>(&ffi);


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

https://reviews.llvm.org/D69764



MyDeveloperDay via Phabricator

unread,
May 24, 2020, 6:27:55 PM5/24/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added a comment.

> I would like to reiterate my discomfort with using East/West as the identifiers here

I'd like to think that I can see it from both angles, @steveire is correct, if I just supply Left/Right then we'll have a request almost immediately for for East/West, Ultimately we'll likely not know what people prefer until its been used. (I'll be interested to do a search in github.com if I ever get this landed)



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

MyDeveloperDay via Phabricator

unread,
May 24, 2020, 6:27:55 PM5/24/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
MyDeveloperDay updated this revision to Diff 265949.
MyDeveloperDay retitled this revision from "[clang-format] Add Left/Right Const fixer capability" to "[clang-format] Add East/West Const fixer capability".
MyDeveloperDay added a comment.

Add more test cases
Cover more template and namespace cases
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

D69764.265949.patch

Manuel Klimek via Phabricator

unread,
May 25, 2020, 7:46:35 AM5/25/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
klimek added inline comments.


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:1378

+**ConstStyle** (``ConstAlignmentStyle``)
+ Different ways to arrange const.
----------------
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.

MyDeveloperDay via Phabricator

unread,
May 25, 2020, 8:19:33 AM5/25/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
MyDeveloperDay added a comment.

Foo<const Foo<int>> P;

// This is needed to trigger the above 'const const' bug above:
#if 0
#else
#endif

produces

Foo<Foo<int> const const> P;

This bug is caused by the way clang-format re-parses the code for each conditional that the preprocessor caused (I actually didn't know it did this, or why)

The more conditional branches we introduce the more this goes wrong...

// 'const >' inserted instead of 'const':
Foo<Foo<int> const const const const> P;

#if 0
#if 1
#else
#endif
#else
#endif

I'm not yet sure of the solution or the reason.

MyDeveloperDay via Phabricator

unread,
May 25, 2020, 1:42:36 PM5/25/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
MyDeveloperDay updated this revision to Diff 266046.
MyDeveloperDay added a comment.

Handle more cases,

Currently on this case is failing, the preprocess lines are causing multiple replacements which means const gets move more than once.

verifyFormat("Foo<const Foo<int>> P;\n#if 0\n#else\n#endif", "Foo<Foo<int> const> P;\n#if 0\n#else\n#endif", Style);
D69764.266046.patch

MyDeveloperDay via Phabricator

unread,
May 25, 2020, 1:42:36 PM5/25/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
MyDeveloperDay planned changes to this revision.

Sam McCall via Phabricator

unread,
May 26, 2020, 8:55:17 AM5/26/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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 :-))

Gašper Ažman

unread,
May 26, 2020, 9:09:28 AM5/26/20
to reviews+D69764+publ...@reviews.llvm.org, mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ste...@kdab.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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.

Aaron Ballman via Phabricator

unread,
May 26, 2020, 10:00:44 AM5/26/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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.

Stephen Kelly via Phabricator

unread,
May 26, 2020, 3:33:05 PM5/26/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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.

MyDeveloperDay via Phabricator

unread,
May 26, 2020, 4:05:41 PM5/26/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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}

MyDeveloperDay via Phabricator

unread,
May 26, 2020, 4:07:09 PM5/26/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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<const Foo<int>> P;\n#if 0\n#else\n#endif", "Foo<Foo<int> 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.

Stephen Kelly via Phabricator

unread,
May 26, 2020, 4:38:31 PM5/26/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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<const Foo<int>> P;
#else
Foo<const Foo<int>> 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.

Aaron Ballman via Phabricator

unread,
May 26, 2020, 5:11:32 PM5/26/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
aaron.ballman added a comment.

In D69764#2055716 <https://reviews.llvm.org/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!

Stephen Kelly via Phabricator

unread,
May 26, 2020, 5:12:49 PM5/26/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
steveire added inline comments.


================
Comment at: clang/lib/Format/EastWestConstFixer.cpp:22
+
+#define DEBUG_TYPE "using-declarations-sorter"
+
----------------
Should this be removed?

MyDeveloperDay via Phabricator

unread,
May 26, 2020, 6:50:40 PM5/26/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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
D69764.266358.patch

MyDeveloperDay via Phabricator

unread,
May 26, 2020, 6:50:44 PM5/26/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
MyDeveloperDay updated this revision to Diff 266360.
MyDeveloperDay added a comment.

rebase with master
D69764.266360.patch

MyDeveloperDay via Phabricator

unread,
May 26, 2020, 6:50:45 PM5/26/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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.

Richard Smith - zygoloid via Phabricator

unread,
May 26, 2020, 7:23:30 PM5/26/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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`.

Stephen Kelly via Phabricator

unread,
May 26, 2020, 7:55:43 PM5/26/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
steveire added a comment.

In D69764#2056104 <https://reviews.llvm.org/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?

MyDeveloperDay via Phabricator

unread,
May 27, 2020, 4:02:49 AM5/27/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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.

MyDeveloperDay via Phabricator

unread,
May 27, 2020, 5:40:02 AM5/27/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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 &DL = F->getParent()->getDataLayout();
^
../polly/lib/Analysis/ScopInfo.cpp:2822:3: warning: code should be clang-formatt
ed [-Wclang-format-violations]
auto const &DL = 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;
^
D69764.266460.patch

Florin Iucha via Phabricator

unread,
May 27, 2020, 8:55:18 AM5/27/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
0x8000-0000 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.

Aaron Ballman via Phabricator

unread,
May 27, 2020, 2:56:37 PM5/27/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
aaron.ballman added a comment.

In D69764#2056104 <https://reviews.llvm.org/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.

Richard Smith - zygoloid via Phabricator

unread,
May 27, 2020, 3:31:05 PM5/27/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
rsmith added a comment.

In D69764#2057945 <https://reviews.llvm.org/D69764#2057945>, @aaron.ballman wrote:

> In D69764#2056104 <https://reviews.llvm.org/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.

MyDeveloperDay via Phabricator

unread,
May 27, 2020, 4:36:38 PM5/27/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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)
D69764.266647.patch

MyDeveloperDay via Phabricator

unread,
May 27, 2020, 5:10:15 PM5/27/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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.

Richard Smith - zygoloid via Phabricator

unread,
May 27, 2020, 6:16:46 PM5/27/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
rsmith added a comment.

In D69764#2058334 <https://reviews.llvm.org/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.)

Marek Kurdej via Phabricator

unread,
May 27, 2020, 7:56:26 PM5/27/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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 &SourceMgr,
+ tooling::Replacements &Fixes,
----------------
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<int> a; const ArrayRef<int> &a;
----------------
"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 would avoid repetition of this statement if you changed while loop into for loop.

Marek Kurdej via Phabricator

unread,
May 27, 2020, 7:56:27 PM5/27/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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.

MyDeveloperDay via Phabricator

unread,
May 28, 2020, 4:35:15 AM5/28/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
MyDeveloperDay planned changes to this revision.

MyDeveloperDay via Phabricator

unread,
May 28, 2020, 6:45:34 AM5/28/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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
D69764.266799.patch

MyDeveloperDay via Phabricator

unread,
May 28, 2020, 6:45:35 AM5/28/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
MyDeveloperDay planned changes to this revision.

MyDeveloperDay via Phabricator

unread,
May 28, 2020, 6:45:37 AM5/28/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
MyDeveloperDay marked 16 inline comments as done.

MyDeveloperDay via Phabricator

unread,
May 28, 2020, 7:18:32 AM5/28/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added inline comments.


================
Comment at: clang/lib/Format/EastWestConstFixer.cpp:130
+
+static void swapFourTokens(const SourceManager &SourceMgr,
+ tooling::Replacements &Fixes,
----------------
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 &SourceMgr,
tooling::Replacements &Fixes, 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";
}
}
```

MyDeveloperDay via Phabricator

unread,
May 28, 2020, 11:07:58 AM5/28/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
MyDeveloperDay marked 3 inline comments as done.
MyDeveloperDay added a comment.

In D69764#2058801 <https://reviews.llvm.org/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?

MyDeveloperDay via Phabricator

unread,
May 28, 2020, 11:08:10 AM5/28/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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
D69764.266883.patch

MyDeveloperDay via Phabricator

unread,
May 28, 2020, 11:08:10 AM5/28/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
MyDeveloperDay planned changes to this revision.
MyDeveloperDay marked 2 inline comments as done.

Marek Kurdej via Phabricator

unread,
May 28, 2020, 11:42:29 AM5/28/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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`.

MyDeveloperDay via Phabricator

unread,
May 28, 2020, 1:56:49 PM5/28/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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)

Aaron Ballman via Phabricator

unread,
May 28, 2020, 3:38:59 PM5/28/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
aaron.ballman added a comment.

In D69764#2058590 <https://reviews.llvm.org/D69764#2058590>, @rsmith wrote:

> In D69764#2058334 <https://reviews.llvm.org/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 to only handle one qualifier and it's been my experience that people usually want their qualifiers grouped together. While I'm sure there are opinions on the ordering *within* the qualifiers where there are multiple different ones, I would guess that there's not a lot of people who would argue that they want their const qualifiers on the left and their volatile qualifiers on the right.

Btw, don't forget that attributes also need to be handled properly. e.g.,
```
long static constexpr unsigned __attribute__((address_space(0))) long const int i = 12;
```
Also, we probably need a test case where the qualifiers have been duplicated, like in this lovely `const` fort:
```
const const const
const int const
const const const i = 12;
```
Do we want that to produce `const const const const const const const const int i = 12;` or `const int i = 12;`?

Sam McCall via Phabricator

unread,
May 29, 2020, 4:03:31 AM5/29/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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.

Stephen Kelly via Phabricator

unread,
May 29, 2020, 2:27:17 PM5/29/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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 <typename T> bool bar(T const &);
template <typename TYPE> bool bat(TYPE const &);
bool bing(A const &);

// const inserted after struct. Does not compile
void fot(struct timespec const t);

// Not transformed
template <typename Type> void tov(typename Type::SubType const tu);

// const inserted after typename. Does not compile
template <typename Type> void tor(typename Type::SubType const &tu);
template <typename Type> void top(typename Type::SubType const *tu);

// const inserted after `TYPE::` (because uppercase?)
template <typename TYPE> 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.

MyDeveloperDay via Phabricator

unread,
May 29, 2020, 3:02:23 PM5/29/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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.

Stephen Kelly via Phabricator

unread,
Jun 13, 2020, 8:37:34 PM6/13/20
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, MarcusLJo...@gmail.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
steveire added a comment.

In D69764#2063798 <https://reviews.llvm.org/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?

Stephen Kelly via Phabricator

unread,
Jan 30, 2021, 3:23:48 PMJan 30
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, the...@gmail.com, con...@protonmail.com, webs...@folling.de, a...@thingdust.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, bhuvanend...@amd.com, llvm-phr...@hazardy.de, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
steveire added a comment.

The idea has been floated to create a new different tool for changes like this (see eg D95168 <https://reviews.llvm.org/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?

MyDeveloperDay via Phabricator

unread,
Jan 31, 2021, 6:53:11 AMJan 31
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, the...@gmail.com, con...@protonmail.com, webs...@folling.de, a...@thingdust.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, bhuvanend...@amd.com, llvm-phr...@hazardy.de, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
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.

Björn Schäpers via Phabricator

unread,
Jan 31, 2021, 11:00:06 AMJan 31
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, llvm-phr...@hazardy.de, the...@gmail.com, con...@protonmail.com, webs...@folling.de, a...@thingdust.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, bhuvanend...@amd.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
HazardyKnusperkeks added a comment.

In D69764#2532666 <https://reviews.llvm.org/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.

Sam McCall via Phabricator

unread,
Jan 31, 2021, 4:26:13 PMJan 31
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, llvm-phr...@hazardy.de, the...@gmail.com, con...@protonmail.com, webs...@folling.de, a...@thingdust.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, bhuvanend...@amd.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
sammccall added a comment.

In D69764#2532666 <https://reviews.llvm.org/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 :-)

Florin Iucha via Phabricator

unread,
Jan 31, 2021, 7:26:21 PMJan 31
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, llvm-phr...@hazardy.de, the...@gmail.com, con...@protonmail.com, webs...@folling.de, a...@thingdust.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, aaron....@gmail.com, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, bhuvanend...@amd.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
0x8000-0000 added a comment.

In D69764#2532952 <https://reviews.llvm.org/D69764#2532952>, @sammccall wrote:

> In D69764#2532666 <https://reviews.llvm.org/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).

Aaron Ballman via Phabricator

unread,
Feb 1, 2021, 7:39:53 AMFeb 1
to mydevel...@gmail.com, kli...@google.com, mitc...@stellarscience.com, samm...@google.com, owen...@gmail.com, kras...@google.com, dja...@google.com, llvm-phr...@hazardy.de, the...@gmail.com, con...@protonmail.com, webs...@folling.de, a...@thingdust.com, ric...@metafoo.co.uk, ste...@kdab.com, gasper...@gmail.com, tho...@chaosfan.de, essex....@gmail.com, Ron...@rondom.de, curd...@gmail.com, flo...@signbit.net, sir.f...@gmail.com, ricc...@gmail.com, ilya....@ericsson.com, devel...@jonas-toth.eu, ll...@meinersbur.de, poll...@googlegroups.com, lebed...@gmail.com, mgo...@gentoo.org, cfe-c...@lists.llvm.org, bhuvanend...@amd.com, pablom...@eskapa.be, mle...@skidmore.edu, blitz...@gmail.com, she...@google.com
aaron.ballman added a reviewer: djasper.
aaron.ballman added a comment.

In D69764#2532413 <https://reviews.llvm.org/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 f