[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

[MyDeveloperDay via Phabricator]

MyDeveloperDay retitled this revision from "[clang-format] Add Left/Right Const (East/West , Before/After) fixer capability" to "[clang-format] Add Left/Right Const fixer capability".
MyDeveloperDay edited the summary of this revision.

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

https://reviews.llvm.org/D69764

[MyDeveloperDay via Phabricator]

MyDeveloperDay planned changes to this revision.

[MyDeveloperDay via Phabricator]

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

I'm returning to this revision which I'd left to think about, this update is really just a rebase and also to remove the dual configuation.

For now I will still with just Left/Right const to avoid confusion.

I'm going to start to review address some of the other review comments next.

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

https://reviews.llvm.org/D69764

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

[MyDeveloperDay via Phabricator]

MyDeveloperDay marked 3 inline comments as done.

[MyDeveloperDay via Phabricator]

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

remove macros from the unit tests

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

https://reviews.llvm.org/D69764

[MyDeveloperDay via Phabricator]

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

Refactor the analyse function to reduce the function size

[MyDeveloperDay via Phabricator]

MyDeveloperDay marked an inline comment as done.

[MyDeveloperDay via Phabricator]

MyDeveloperDay planned changes to this revision.

[Stephen Kelly via Phabricator]

steveire added a comment.

I like the approach of using clang-format to implement this. It's much faster than a `clang-tidy` approach.

The broader C++ community has already chosen `East`/`West` and it has momentum. If you choose `Left`/`Right` now, you will get pressure to add `East`/`West` in the future, which means we'll have the synonyms we want to avoid.

The broader C++ community already has understanding of `East`/`West`. Trying to change that now should be out of scope for this patch. This patch should use `East`/`West`.

I ran this on a large codebase and discovered some problems with this patch. Given this `.clang-format` file:

BasedOnStyle: LLVM
PointerAlignment: Left
ConstStyle: Right

I get the failures as commented here:

template <typename T> struct Foo {};

template <> struct Foo<int> {
static const int bat;
static const int fn();
};

// 'const' inserted in wrong place:
int const Foo<int>::bat = 0;
// 'const const' inserted in wrong place:
int const Foo<int>::fn() {
return 0;
}

void foo() {
Foo<Foo<int>> ffi;
// '* const' inserted instead of 'const':
const Foo<Foo<int>>* p = const_cast<const Foo<Foo<int>>*>(&ffi);
}

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

template <typename T>
// 'const const' inserted instead of 'const':
void fn(const Foo<T>& i);

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

namespace ns {
struct S {};
} // namespace ns

// not modified:
void fns(const ns::S& s);

Some of these (eg the `int Foo<int>::bat const = 0`) are the kinds of breakages `clang-format` so far avoids by not reordering tokens, as @sammccall was saying. I don't know if the `clang-format` parser is smart enough to not cause those bugs.

Thanks for working on this!

[MyDeveloperDay via Phabricator]

MyDeveloperDay added a comment.

In D69764#2050226 <https://reviews.llvm.org/D69764#2050226>, @steveire wrote:

> I like the approach of using clang-format to implement this. It's much faster than a `clang-tidy` approach.
>
> The broader C++ community has already chosen `East`/`West` and it has momentum. If you choose `Left`/`Right` now, you will get pressure to add `East`/`West` in the future, which means we'll have the synonyms we want to avoid.
>
> The broader C++ community already has understanding of `East`/`West`. Trying to change that now should be out of scope for this patch. This patch should use `East`/`West`.
>
> I ran this on a large codebase and discovered some problems with this patch. Given this `.clang-format` file:

Thank you for this feedback @steveire, to be honest I agree, I didn't want to waste time arguing about the naming for now so simply gave in. Supporting multiple words from the outset also felt wrong, maybe we can spin around later towards the end of the review if there is more of a concencus on naming being the other way.

Thank you also for the failure scenarios I will add them as tests as I try to improve this further.

I think there was a suggestion that somehow this should cover all forms of identifier ordering but I actually think that is going to be incredibly complex especially if that configuration was completely dynamic and supported custom types and macros

For now let me pursue fixes for the cases you have identified.

[Stephen Kelly via Phabricator]

steveire added a comment.

In D69764#2050538 <https://reviews.llvm.org/D69764#2050538>, @MyDeveloperDay wrote:

> In D69764#2050226 <https://reviews.llvm.org/D69764#2050226>, @steveire wrote:
>
> > I like the approach of using clang-format to implement this. It's much faster than a `clang-tidy` approach.
> >
> > The broader C++ community has already chosen `East`/`West` and it has momentum. If you choose `Left`/`Right` now, you will get pressure to add `East`/`West` in the future, which means we'll have the synonyms we want to avoid.
> >
> > The broader C++ community already has understanding of `East`/`West`. Trying to change that now should be out of scope for this patch. This patch should use `East`/`West`.
> >
> > I ran this on a large codebase and discovered some problems with this patch. Given this `.clang-format` file:
>
>
> Thank you for this feedback @steveire, to be honest I agree, I didn't want to waste time arguing about the naming for now so simply gave in. Supporting multiple words from the outset also felt wrong, maybe we can spin around later towards the end of the review if there is more of a concencus on naming being the other way.

It seems to be only Aaron who is against East/West. And his objection doesn't seem to be considerate of the broader consensus. I'm sure it's no problem to use those names.

> Thank you also for the failure scenarios I will add them as tests as I try to improve this further.
>
> I think there was a suggestion that somehow this should cover all forms of identifier ordering but I actually think that is going to be incredibly complex especially if that configuration was completely dynamic and supported custom types and macros
>
> For now let me pursue fixes for the cases you have identified.

Great, here's a few more which don't currently get converted :) :

void autofn() {
const auto i = 0;
const auto& ir = i;
const auto* ip = &i;

[Aaron Ballman via Phabricator]

aaron.ballman added a comment.

In D69764#2050226 <https://reviews.llvm.org/D69764#2050226>, @steveire wrote:

> I like the approach of using clang-format to implement this. It's much faster than a `clang-tidy` approach.
>
> The broader C++ community has already chosen `East`/`West` and it has momentum. If you choose `Left`/`Right` now, you will get pressure to add `East`/`West` in the future, which means we'll have the synonyms we want to avoid.
>
> The broader C++ community already has understanding of `East`/`West`. Trying to change that now should be out of scope for this patch. This patch should use `East`/`West`.

As a member of the C++ committee, I'm aware that any statements about what the broader C++ community understands are not to be taken too seriously; none of us know what ~5M people understand or don't.

I would like to reiterate my discomfort with using East/West as the identifiers here. The purpose to this functionality is to decide whether to put qualifiers before or after the base type -- use of east/west to describe relative locations like these is not idiomatic in the same way as left/right or before/after (e.g., I'm not east-handed and you don't put the cart to the west of the horse). Qualifiers have been in C++ for ~40 years and the notion of east/west terminology is a very recent evolution by comparison. Also, clang-format is not a formatter for C++ alone and the east/west terminology is likely even less-known to users of the other languages despite also having qualifiers that need formatting (e.g., C and Obj). I think these are valid technical concerns with the patch that we should not hand-wave away as matters of preference.



================
Comment at: clang/lib/Format/Format.cpp:2547

+ if (Style.isCpp() || Style.Language == FormatStyle::LK_ObjC) {
+ if (Style.ConstStyle != FormatStyle::CS_Leave)
----------------
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.

[MyDeveloperDay via Phabricator]

MyDeveloperDay updated this revision to Diff 365276.
MyDeveloperDay retitled this revision from "[clang-format] Add East/West Const fixer capability" to "[clang-format] Add Left/Right Const fixer capability".
MyDeveloperDay added a comment.

Remove East/West terminology for inclusivity

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

https://reviews.llvm.org/D69764

Files:
clang/docs/ClangFormatStyleOptions.rst
clang/docs/ReleaseNotes.rst
clang/include/clang/Format/Format.h
clang/lib/Format/CMakeLists.txt

clang/lib/Format/Format.cpp
clang/lib/Format/LeftRightConstFixer.cpp
clang/lib/Format/LeftRightConstFixer.h
clang/tools/clang-format/ClangFormat.cpp
clang/unittests/Format/FormatTest.cpp

[MyDeveloperDay via Phabricator]

MyDeveloperDay updated this revision to Diff 365456.
MyDeveloperDay marked 2 inline comments as done.
MyDeveloperDay added a comment.

- Add support for both const and volatile alignment
- change `ConstPlacement` to `CVQualifierAlignment`
- add `CVQualifierOrder` to allow control over `const volatile

[Björn Schäpers via Phabricator]

HazardyKnusperkeks added a comment.

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

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



================
Comment at: clang/lib/Format/Format.cpp:2890-2892
+ if (Style.CVQualifierAlignment == FormatStyle::CVQAS_Left) {
+ std::reverse(Order.begin(), Order.end());
+ }
----------------



================
Comment at: clang/lib/Format/Format.cpp:2896
+ // will be out of scope at construction.
+ for (std::string CVQualifier : Order) {
+ if (CVQualifier == "const") {
----------------
I don't know if we should remove the braces here too.

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

[MyDeveloperDay via Phabricator]

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

- elide the braces
- use references

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

https://reviews.llvm.org/D69764

[MyDeveloperDay via Phabricator]

MyDeveloperDay added a comment.

In D69764#2936827 <https://reviews.llvm.org/D69764#2936827>, @HazardyKnusperkeks wrote:

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

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

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

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

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

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

[MyDeveloperDay via Phabricator]

MyDeveloperDay added a comment.

> In D69764#2936827 <https://reviews.llvm.org/D69764#2936827>, @HazardyKnusperkeks wrote:
>
>> First off, I think it should be configured in a different way, to prepare the path for also formatting static, inline, etc.

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

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

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

[MyDeveloperDay via Phabricator]

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

- Add CVQualifierOrder configuration validation and unit tests

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

https://reviews.llvm.org/D69764

[MyDeveloperDay via Phabricator]

MyDeveloperDay marked 3 inline comments as done.

[MyDeveloperDay via Phabricator]

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

- Rename the ConstFixer to QualifierAligmentFixer (as now we handle more than just const) and in preparation for handling others

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

https://reviews.llvm.org/D69764

Files:
clang/docs/ClangFormatStyleOptions.rst
clang/docs/ReleaseNotes.rst
clang/include/clang/Format/Format.h
clang/lib/Format/CMakeLists.txt
clang/lib/Format/Format.cpp

clang/lib/Format/QualifierAlignmentFixer.cpp
clang/lib/Format/QualifierAlignmentFixer.h
clang/tools/clang-format/ClangFormat.cpp
clang/unittests/Format/FormatTest.cpp

[MyDeveloperDay via Phabricator]

MyDeveloperDay added a comment.

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

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

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

So we would be able to have:

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

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

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

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

Left would by default define:

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

Right would define:

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

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

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

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

Does anyone have any thoughts on this approach?

[Björn Schäpers via Phabricator]

HazardyKnusperkeks added a comment.

In D69764#2938973 <https://reviews.llvm.org/D69764#2938973>, @MyDeveloperDay wrote:

> With the introduction of `CVQualifierOrder` the need for `CVQualifierAlignment:` doesn't make so much sense
>
> CVQualifierAlignment: Left
> CVQualifierOrder: [ "inline", "static", "volatile", "const" ]
>

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

>
> So we would be able to have:
>
> CVQualifierOrder: [ "inline", "static", "volatile", "<type>", "const", "restrict" ]
>
> By doing so `inline,static,and volatile` are Left and `const,restrict` are Right and so a single setting of Left and Right isn't applicable.
>
> The whole feature could then be determined ONLY using the `CVQualifierOrder` however it doesn't feel like there is a good way to say "Don't change order" (unless of course I use an empty `CVQualifierOrder` list), but this doesn't feel very off by default enough, I like the explicitness of `Leave`.
>

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

>
> Left would by default define:
>
> CVQualifierOrder: [ "inline", "static", "volatile", "const", "restrict", "<type>"]
>
> Right would define:
>
> CVQualifierOrder: [ "inline", "static", "<type>", "volatile", "const", "restrict" ]
>
> (we might want to discuss what these defaults should be, I'm not a language lawyer!)
>
> I realise "inline" and "static" are to the left in the Right alignment, but I'm not sure them being to the right is even legal (its certainly not common) or what is most likely wanted.
>
> I also think if a `CVQualifierOrder` is defined and the `CVQualifierAlignment` is not `Custom` that should be a configuration error to ensure there isn't any ambiguity
>
> Does anyone have any thoughts on this approach?

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

================
Comment at: clang/lib/Format/Format.cpp:1494

+ // If its empty then it means don't do anything.
+ if (Style->CVQualifierOrder.empty()) {
+ return true;
----------------
Braces :)


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:32
+ if (Err) {
+ llvm::errs() << "Error while rearranging const : "
+ << llvm::toString(std::move(Err)) << "\n";
----------------
Qualifier?


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:140
+ // We only need to think about streams that begin with const.
+ if (!Tok->is(CVQualifierType)) {
+ return Tok;
----------------
More Braces (follow)


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.h:24
+class QualifierAlignmentFixer : public TokenAnalyzer {
+ std::string CVQualifier;
+
----------------
I would go for only Qualifier.

[Marek Kurdej via Phabricator]

curdeius added a comment.

In D69764#2939037 <https://reviews.llvm.org/D69764#2939037>, @HazardyKnusperkeks wrote:

> In D69764#2938973 <https://reviews.llvm.org/D69764#2938973>, @MyDeveloperDay wrote:
>
>> I'm leaning toward introducing <type> into the `CVQualifierOrder` allowing for some qualifiers to be Left of the type and some to be Right (this is not dissimilar to what was discussed some time ago, and similar to the concept of the resharper image above)
>

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

I like both proposals.



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

+**CVQualifierAlignment** (``CVQualifierAlignmentStyle``)
+ Different ways to arrange const/volatile qualifiers.
----------------
Why not just `Qualifier` to allow future additions for other qualifiers?
Technically, `static` or `inline` are not qualifiers, but that maybe is clear enough?

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


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:2107
+ * ``CVQAS_Leave`` (in configuration: ``Leave``)
+ Don't change cvqualifier to either Left or Right alignment
+
----------------
Nit: `cvqualifier` seems ugly, here and below.


================
Comment at: clang/lib/Format/Format.cpp:2908-2909
+ // Depending on the placement style of left or right you need
+ // To iterate forward or backward though the order list as qualifier
+ // can push though each other.
+ // Copy the very small list and reverse the order if left aligned.
----------------
Nit: typos.


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:201
+ }
+ Next = Next->Next;
+ }
----------------
That will access a nullptr if `Next` in the previous `if` was null. Is that possible BTW? Can we add a test for this? Maybe some possibly invalid code that starts has identifier and coloncolon but doesn't have a template opener? (maybe just `const std::Foo`)?

[MyDeveloperDay via Phabricator]

MyDeveloperDay added a subscriber: asb.
MyDeveloperDay added a comment.

> In D69764#2939037 <https://reviews.llvm.org/D69764#2939037>, @HazardyKnusperkeks wrote:

> I would like to remove the CV, only QualifierOrder.

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

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

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

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

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

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

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

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

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

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

================
Comment at: clang/docs/ClangFormatStyleOptions.rst:2107
+ * ``CVQAS_Leave`` (in configuration: ``Leave``)
+ Don't change cvqualifier to either Left or Right alignment
+
----------------

curdeius wrote:
> Nit: `cvqualifier` seems ugly, here and below.

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

================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:140
+ // We only need to think about streams that begin with const.
+ if (!Tok->is(CVQualifierType)) {
+ return Tok;
----------------

HazardyKnusperkeks wrote:
> More Braces (follow)
Uh! this is my own style bleeding through... its why I need clang-format to remove them for me! ;-)

================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:201
+ }
+ Next = Next->Next;
+ }
----------------

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

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

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

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

================
Comment at: clang/lib/Format/QualifierAlignmentFixer.h:24
+class QualifierAlignmentFixer : public TokenAnalyzer {
+ std::string CVQualifier;
+
----------------

HazardyKnusperkeks wrote:
> I would go for only Qualifier.

Do you think everywhere now too? including the options?

[Aaron Ballman via Phabricator]

aaron.ballman added a comment.

In D69764#2938973 <https://reviews.llvm.org/D69764#2938973>, @MyDeveloperDay wrote:

> With the introduction of `CVQualifierOrder` the need for `CVQualifierAlignment:` doesn't make so much sense
>
> CVQualifierAlignment: Left
> CVQualifierOrder: [ "inline", "static", "volatile", "const" ]
>

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

> So we would be able to have:
>
> CVQualifierOrder: [ "inline", "static", "volatile", "<type>", "const", "restrict" ]
>
> By doing so `inline,static,and volatile` are Left and `const,restrict` are Right and so a single setting of Left and Right isn't applicable.
>
> The whole feature could then be determined ONLY using the `CVQualifierOrder` however it doesn't feel like there is a good way to say "Don't change order" (unless of course I use an empty `CVQualifierOrder` list), but this doesn't feel very off by default enough, I like the explicitness of `Leave`.
>
> My gut feeling is to make `Left` and `Right` options define a predetermined CVQualifierOrder, and introduce `Custom` as a new CVQualifierAlignment setting which would require you to define the Order, this has the advantage of driving a sort of semi standard as to what we mean by `Left` and `Right`
>
> Left would by default define:
>
> CVQualifierOrder: [ "inline", "static", "volatile", "const", "restrict", "<type>"]
>
> Right would define:
>
> CVQualifierOrder: [ "inline", "static", "<type>", "volatile", "const", "restrict" ]
>
> (we might want to discuss what these defaults should be, I'm not a language lawyer!)
>
> I realise "inline" and "static" are to the left in the Right alignment, but I'm not sure them being to the right is even legal (its certainly not common) or what is most likely wanted.
>
> I also think if a `CVQualifierOrder` is defined and the `CVQualifierAlignment` is not `Custom` that should be a configuration error to ensure there isn't any ambiguity
>
> Does anyone have any thoughts on this approach?

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

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

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

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

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

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

[MyDeveloperDay via Phabricator]

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

- Remove the "CV" from the options
- Add support for ordering static/inline/const/volatile around the type, allowing for mixing both left and right alignment at the same time

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



- Add "Custom" format to allow use of QualifierOrder (may want to change the `Qualifier` name as its now both Qualifiers and Specifiers)
- Add Parse errors and test to check for unsupported specifiers/qualifiers, duplicates,missing 'type' field and empty Order lists
- Add Defaults for Left and Right Alignment (may need to discuss suitable defaults).
- Removal of excess braces (if only clang-format could do that for us!!)

NOTE: Known issue:

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

For the following example

inline volatile int const c;

with the .clang-format file of:

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

I get the following:

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

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

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

Its likely I am switching the order from:

inline volatile int const c;

to

volatile inline int const c;

and then back to

inline volatile int const c;

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

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

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

https://reviews.llvm.org/D69764

[MyDeveloperDay via Phabricator]

MyDeveloperDay marked 5 inline comments as done.

[Stephen Kelly via Phabricator]

steveire added a comment.

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

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

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

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

[Marek Kurdej via Phabricator]

curdeius added a comment.

In D69764#2996365 <https://reviews.llvm.org/D69764#2996365>, @steveire wrote:

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

Thank you for your comment.

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

My 2 cents.

[Aaron Ballman via Phabricator]

aaron.ballman added a comment.

In D69764#2996365 <https://reviews.llvm.org/D69764#2996365>, @steveire wrote:

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

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

[MyDeveloperDay via Phabricator]

MyDeveloperDay added a comment.

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

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

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

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

[MyDeveloperDay via Phabricator]

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

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

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

Still WIP but getting much closer.

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

https://reviews.llvm.org/D69764

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

clang/unittests/Format/CMakeLists.txt
clang/unittests/Format/FormatTest.cpp
clang/unittests/Format/QualifierFixerTest.cpp

[Aaron Ballman via Phabricator]

aaron.ballman added a comment.

In D69764#2997910 <https://reviews.llvm.org/D69764#2997910>, @MyDeveloperDay wrote:

> I think the relevance of Left/Right East/West as a setting is less important, as its more about an ordering, allowing some tokens to go to the Left and some to the Right of the "type"
>
> QualifierAlignment: Custom
> QualifierOrder: [ inline, static, type, const, volatile, restrict ]
>
> While I currently include the Left/Right for some form of completeness I sort of feel i should kill them before we landing this, as I think even my defaults are a little subjective. (unless we can get concencus), hence I don't feel the need to bring back up the Left/Right/East/West debate. If its still a problem later we can add them.

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

[MyDeveloperDay via Phabricator]

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

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

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

https://reviews.llvm.org/D69764

[MyDeveloperDay via Phabricator]

MyDeveloperDay marked 3 inline comments as done.

MyDeveloperDay added inline comments.

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

+**CVQualifierAlignment** (``CVQualifierAlignmentStyle``)
+ Different ways to arrange const/volatile qualifiers.
----------------

curdeius wrote:
> Why not just `Qualifier` to allow future additions for other qualifiers?
> Technically, `static` or `inline` are not qualifiers, but that maybe is clear enough?
>
> Other possibility, `Specifier` (as in "inline specifier")?
> OTOH, `Keyword` may be too generic.

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

[MyDeveloperDay via Phabricator]

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

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

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

F19136751: image.png <https://reviews.llvm.org/F19136751>

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

https://reviews.llvm.org/D69764

Files:
clang/docs/ClangFormatStyleOptions.rst
clang/docs/ReleaseNotes.rst
clang/docs/tools/dump_format_style.py

[MyDeveloperDay via Phabricator]

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

Missed dump_format_style.py update

[MyDeveloperDay via Phabricator]

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

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

[Björn Schäpers via Phabricator]

HazardyKnusperkeks added a comment.

Really nice!

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



================
Comment at: clang/include/clang/Format/Format.h:1863-1864
+ /// \warning
+ /// ``QualifierAlignment`` COULD lead to incorrect code generation, use with
+ /// caution.
+ /// \endwarning
----------------
I would drop that use with caution. I think without the warning is big enough, and with it's too much.


================
Comment at: clang/lib/Format/Format.cpp:2938
namespace internal {
+
std::pair<tooling::Replacements, unsigned>
----------------
Nit: Unrelated (and unnecessary) change.


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:28
+
+static void replaceToken(const SourceManager &SourceMgr,
+ tooling::Replacements &Fixes,
----------------
Out of curiosity, on what premise do you choose a static member function that is only used in this file or a local free function? I would always choose the latter (with an anon namespace), to keep the header smaller.


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:57
+
+ std::string NewText = " " + Qualifier + " ";
+ NewText += Next->TokenText;
----------------
Does not need to be addressed here, but does LLVM have a string builder or so? This produces a lot of (de-)allocations.


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:129
+
+bool LeftRightQualifierAlignmentFixer::isQualifierOrType(
+ const FormatToken *Tok) {
----------------
I would prefer to match the order of functions in the header with the order in the cpp.


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:161
+ return Tok;
+ FormatToken *Const = Tok;
+
----------------
This name is legacy from just `const volatile` formatting?


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:347
+ QualifierToken);
+ else if (Alignment == FormatStyle::QAS_Left)
+ Tok = analyzeLeft(SourceMgr, Keywords, Fixes, Tok, Qualifier,
----------------
Only else and assert? It does nothing if Alignment is something different, or?


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:366
+ // Split the Order list by type and reverse the left side
+ bool left = true;
+ for (const auto &s : Order) {
----------------
Couldn't you just
```
LeftOrder.assign(std::reverse_iterator(type) /*maybe with a next or prev, haven't thought too much about it*/, Order.rend());
RightOrder.assign(std::next(type), Order.end());
```
?

================
Comment at: clang/lib/Format/QualifierAlignmentFixer.h:24
+

+typedef std::function<std::pair<tooling::Replacements, unsigned>(
+ const Environment &)>
----------------
I don't know what the LLVM style is on that, but I prefer `using` anytime over `typedef`.


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.h:30
+ // Left to Right ordering requires multiple passes
+ SmallVector<AnalyzerPass, 8> Passes;
+ StringRef &Code;
----------------
Has the 8 some meaning? Then maybe give it a name. Or is it just coincidence that you repeat the 8 for QualifierTokens?


================
Comment at: clang/unittests/Format/FormatTest.cpp:18233

+#define FAIL_PARSE(TEXT, FIELD, VALUE) \
+ EXPECT_NE(0, parseConfiguration(TEXT, &Style).value()); \
----------------
Unused?

[MyDeveloperDay via Phabricator]

MyDeveloperDay updated this revision to Diff 373795.
MyDeveloperDay marked 11 inline comments as done.
MyDeveloperDay added a comment.

Address review comments.

- reorder functions to match header

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

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

https://reviews.llvm.org/D69764

[MyDeveloperDay via Phabricator]

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

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

[MyDeveloperDay via Phabricator]

MyDeveloperDay added inline comments.

================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:28
+
+static void replaceToken(const SourceManager &SourceMgr,
+ tooling::Replacements &Fixes,
----------------

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

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

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

================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:57
+
+ std::string NewText = " " + Qualifier + " ";
+ NewText += Next->TokenText;
----------------

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

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

================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:129
+
+bool LeftRightQualifierAlignmentFixer::isQualifierOrType(
+ const FormatToken *Tok) {
----------------

HazardyKnusperkeks wrote:
> I would prefer to match the order of functions in the header with the order in the cpp.

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

================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:347
+ QualifierToken);
+ else if (Alignment == FormatStyle::QAS_Left)
+ Tok = analyzeLeft(SourceMgr, Keywords, Fixes, Tok, Qualifier,
----------------

HazardyKnusperkeks wrote:
> Only else and assert? It does nothing if Alignment is something different, or?

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

================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:366
+ // Split the Order list by type and reverse the left side
+ bool left = true;
+ for (const auto &s : Order) {
----------------

HazardyKnusperkeks wrote:
> Couldn't you just
> ```
> LeftOrder.assign(std::reverse_iterator(type) /*maybe with a next or prev, haven't thought too much about it*/, Order.rend());
> RightOrder.assign(std::next(type), Order.end());
> ```
> ?

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

================
Comment at: clang/lib/Format/QualifierAlignmentFixer.h:24
+
+typedef std::function<std::pair<tooling::Replacements, unsigned>(
+ const Environment &)>
----------------

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

I was actually just following what is in Format.cpp

================
Comment at: clang/lib/Format/QualifierAlignmentFixer.h:30
+ // Left to Right ordering requires multiple passes
+ SmallVector<AnalyzerPass, 8> Passes;
+ StringRef &Code;
----------------

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

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

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

[MyDeveloperDay via Phabricator]

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

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

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

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

https://reviews.llvm.org/D69764

[MyDeveloperDay via Phabricator]

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

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

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

[Björn Schäpers via Phabricator]

HazardyKnusperkeks added inline comments.


================

Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:57
+
+ std::string NewText = " " + Qualifier + " ";
+ NewText += Next->TokenText;
----------------

MyDeveloperDay wrote:
> HazardyKnusperkeks wrote:
> > Does not need to be addressed here, but does LLVM have a string builder or so? This produces a lot of (de-)allocations.
> I'm not a massive fan of this either, but I'm unsure if LLVM has anything, I think getting functional first is important, we can go for fast if someone can point out a better pattern.

Full support for that.

================
Comment at: clang/lib/Format/QualifierAlignmentFixer.h:30
+ // Left to Right ordering requires multiple passes
+ SmallVector<AnalyzerPass, 8> Passes;
+ StringRef &Code;
----------------

MyDeveloperDay wrote:
> HazardyKnusperkeks wrote:
> > Has the 8 some meaning? Then maybe give it a name. Or is it just coincidence that you repeat the 8 for QualifierTokens?
> For SmallVector this is basically a "ShortStringOptimization" for vector i.e. its stack allocated until the vector goes over 8, I have 7 qualifiers, and I wanted it an order of 2 so hence 8 (its shouldn't grow (and heap allocation) unless we define more qualifier types (this only supports what I say for now)
>
> I think the use of a literal is quite common in this case, its really just a hint, I think its ok to use without it being a variable.

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

Nothing I would block that change for.


================
Comment at: clang/unittests/Format/QualifierFixerTest.cpp:573
+ // The Default
+ EXPECT_EQ(Style.QualifierOrder.size(), 5);
+ EXPECT_EQ(Style.QualifierOrder[0], "inline");
----------------
This one would suffice, since we add the values within the test now. But actually I would go for Style.QualifierOrder = {"inline",...}; and no EXPECTs needed.


================
Comment at: clang/unittests/Format/QualifierFixerTest.cpp:855
+
+ Style.TypenameMacros.push_back("HRESULT");
+ Style.TypenameMacros.push_back("DWORD");
----------------
This is not correct, the documentation of TypenameMacros says
```These are expected to be macros of the form:

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

[MyDeveloperDay via Phabricator]

MyDeveloperDay updated this revision to Diff 374294.
MyDeveloperDay marked 3 inline comments as done.
MyDeveloperDay added a comment.

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

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

https://reviews.llvm.org/D69764

[MyDeveloperDay via Phabricator]

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

Use better vector initialization

[Björn Schäpers via Phabricator]

HazardyKnusperkeks added inline comments.


================

Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:449
+ return false;
+ if (Tok->is(TT_TypenameMacro))
+ return false;
----------------
I think this is still right, because it is a type (according to the configuration).


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.h:16
+
+#ifndef LLVM_CLANG_LIB_FORMAT_LeftRightQualifierAlignmentFixer_H
+#define LLVM_CLANG_LIB_FORMAT_LeftRightQualifierAlignmentFixer_H
----------------
This clang-tidy isn't addressed.


================
Comment at: clang/unittests/Format/QualifierFixerTest.cpp:122
+
+}; // namespace
+
----------------
In addition to clang-tidies message: It is unnecessary.

[MyDeveloperDay via Phabricator]

MyDeveloperDay updated this revision to Diff 374455.

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

Fix clang-format and clang-tidy issues

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

https://reviews.llvm.org/D69764

[MyDeveloperDay via Phabricator]

MyDeveloperDay added inline comments.

================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:449
+ return false;
+ if (Tok->is(TT_TypenameMacro))
+ return false;
----------------

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

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

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

[Björn Schäpers via Phabricator]

HazardyKnusperkeks accepted this revision.
HazardyKnusperkeks added a comment.
This revision is now accepted and ready to land.

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

================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:449
+ return false;
+ if (Tok->is(TT_TypenameMacro))
+ return false;
----------------

MyDeveloperDay wrote:
> HazardyKnusperkeks wrote:
> > I think this is still right, because it is a type (according to the configuration).
> yeah, let's do this a different way after we have landed this, I was thinking about having a list of "Types" that users could specify that would help us classify tok::identifiers a little better (could help with some of the casting issues we've seen before)
>
> I notice in projects like Linux they make heavy use of these ForeachMacros and StatementMacros, I don't want to steal another list I'd rather we had one that was very specific to types, I think this could help

We can do that.

[MyDeveloperDay via Phabricator]

MyDeveloperDay added a comment.

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

[MyDeveloperDay via Phabricator]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa44ab1702539: [clang-format] Add Left/Right Const fixer capability (authored by MyDeveloperDay).

Repository:
rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69764

[Nemanja Ivanovic via Phabricator]

nemanjai added a comment.

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

[MyDeveloperDay via Phabricator]

MyDeveloperDay added a comment.

In D69764#3019409 <https://reviews.llvm.org/D69764#3019409>, @nemanjai wrote:

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

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

[Simon Giesecke via Phabricator]

simon.giesecke added inline comments.


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3233
+
+ ``QualifierAlignment`` COULD lead to incorrect code generation.
+
----------------
This is pretty unclear, for a number of reasons:
* First, this probably only refers to a setting other than `QAS_Leave`?
* Second, "lead to incorrect code generation" seems to skip a step. In the first place, this seems to imply that a setting other than `QAS_Leave` might change the semantics of the source code.
* Third, it's not clear to me when this would happen, and how likely that is. Does this mean "Non-default settings are experimental, and you shouldn't use this in production?" or rather "Under rare circumstances that are unlikely to happen in real code, a non-default setting might change semantics." At the minimum, could this give some example(s) when this happens?

[Björn Schäpers via Phabricator]

HazardyKnusperkeks added inline comments.

================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3233
+
+ ``QualifierAlignment`` COULD lead to incorrect code generation.
+
----------------

simon.giesecke wrote:
> This is pretty unclear, for a number of reasons:
> * First, this probably only refers to a setting other than `QAS_Leave`?
> * Second, "lead to incorrect code generation" seems to skip a step. In the first place, this seems to imply that a setting other than `QAS_Leave` might change the semantics of the source code.
> * Third, it's not clear to me when this would happen, and how likely that is. Does this mean "Non-default settings are experimental, and you shouldn't use this in production?" or rather "Under rare circumstances that are unlikely to happen in real code, a non-default setting might change semantics." At the minimum, could this give some example(s) when this happens?

* Yes.
* Yes, it might change the semantics, that was the content of a huge discussion here in the review and on the mailing list. Consensus was found that non whitespace changes are acceptable with a big warning and off by default.
* The latter, if we would have an example at hand in which cases it wouldn't work, we would fix that and not give it as an example. So to the best of our knowledge it does not break anything.

The warning text however might be improved.

[MyDeveloperDay via Phabricator]

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

================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3233
+
+ ``QualifierAlignment`` COULD lead to incorrect code generation.
+
----------------

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

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

```
#define INTPTR int *

const INTPTR a;
```

In this sense moving const changes this from

```
const int * a;
```
to

```
int * const a;
```

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

I guess if someone does

```
#define intptr int *
```

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

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

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

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

[MyDeveloperDay via Phabricator]

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

I'm not a wordsmith but how about D110801: [clang-format] [docs] [NFC] improve clarity in the QualifierAlignment warning <https://reviews.llvm.org/D110801>

[Simon Giesecke via Phabricator]

simon.giesecke added inline comments.

================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3233
+
+ ``QualifierAlignment`` COULD lead to incorrect code generation.
+
----------------

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

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

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

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

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