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

Tue Jul 13 11:45:44 PDT 2021

HazardyKnusperkeks added a comment.

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

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

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

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

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

================
Comment at: clang/lib/Format/EastWestConstFixer.cpp:293
+      Tok->Next->Next->Next && Tok->Next->Next->Next->is(tok::kw_const)) {
+    swapFourTokens(SourceMgr, Fixes, Tok, Tok->Next, Tok->Next, Tok->Next->Next->Next,
+                         /*West=*/true);
----------------
aaron.ballman wrote:
> curdeius wrote:
> > 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;`?
I would be in favor of just `const int`.

================
Comment at: clang/lib/Format/Format.cpp:1260
     GoogleStyle.CommentPragmas = "(taze:|^/[ \t]*<|tslint:|@see)";
+    GoogleStyle.ConstPlacement = FormatStyle::CS_Leave;
     // TODO: enable once decided, in particular re disabling bin packing.
----------------
These are not needed if the LLVM style is already `Leave`.

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

https://reviews.llvm.org/D69764