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

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Nov 4 05:37:23 PST 2019


aaron.ballman added a comment.

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

> In D69764#1732235 <https://reviews.llvm.org/D69764#1732235>, @aaron.ballman wrote:
>
> > I like the functionality, but am slightly opposed to using "east/west" terminology -- that's not a ubiquitous phrase and it takes a bit of thinking before it makes sense. I think "left/right" is likely to be more universally understood.
> >
> > Also, should this apply to other qualifiers like `volatile` or `restrict`? If so, the name `ConstStyle` should probably be `CVQualifierStyle` or something else.
>
>
> East/West I think is a term coined I think by Simon Brand (@TartanLlama https://twitter.com/tartanllama) or maybe Dan Saks before, I did a quick check on github and both "west const","east const" ,"left const" and "right const" are all used in commit messages, I'd really like to keep the East/West just because that's the name of the rebellion (not affiliated!), but we could alias to `Left` and `Right` (like we sometimes do for true/false when we overload the options), I've also seen const `Before` and const `After` used. again if people don't think its too much overload I'd be happy to add those if it means people get the gist a little clearer.


I think east/west is more of a C++ community term than a C community one (and it's a newer phrase). I'd be fine with before/after as well as left/right, with an alias for east/west, but I'd like the canonical name to not be east/west.

> I'd probably agree with the CVQualifierStyle although not such a nice command-line name, what also concerns me is sometimes you see `const volatile type` and sometimes `volatile const type` sometimes `type const volatile` almost feels like volatile might needs its own VolatileStyle allowing
> 
>   const volatile int   (ConstStyle: West,  VolatileStyle: East)
>   volatile const int   (ConstStyle: West,  VolatileStyle: West)
>   int const volatile   (ConstStyle: East,  VolatileStyle: East)
>   int volatile const   (ConstStyle: West,  VolatileStyle: West)
> 
> 
> Whilst I agree one option would be better, I think we'll end up being asked for more flexible support, such is the way for people to want their own style.

If we wanted to add more flexible support, we could do so with a placeholder syntax, like `CVQualifierStyle: volatile const %ident% restrict` or some such, so I don't think such a name fully closes the design space. However, if we do want to stick with separate styles for each of these, we should have explicit comments and tests showing this should only work for `const` qualifiers.


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

https://reviews.llvm.org/D69764





More information about the cfe-commits mailing list