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

Wed Jun 2 15:17:36 PDT 2021

davidstone added a comment.

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

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

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

  const int * pointer_to_const = nullptr;
  int const * also_pointer_to_const = nullptr;
  int * const const_pointer_to_mutable = nullptr;

  constexpr int * constexpr_pointer_to_mutable = nullptr;
  int constexpr * also_constexpr_pointer_to_mutable = nullptr;
  int * constexpr invalid = nullptr;

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

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

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

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

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

================
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:
> 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).
I would expect whatever option we decide on here to apply to both cv qualifiers. I don't think anyone wants to split `const` and `volatile`.

As to whether we need to preserve relative ordering of `const` and `volatile` or allow users to specify the order, I don't think that matters much. I've never seen a discussion about the relative order of the two -- when both exist, I've always seen `const volatile`. Given that no users seem to be clamoring for that as a customization point, I would suggest `const volatile` when both are present, put on whichever side the user configures. Basically, don't give users more knobs than they actually want to turn.

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

https://reviews.llvm.org/D69764