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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D69764#2058590 <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;`?


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

https://reviews.llvm.org/D69764