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

[MyDeveloperDay via Phabricator via cfe-commits]

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

In D69764#2058801 <https://reviews.llvm.org/D69764#2058801>, @curdeius wrote:

> One last thought, how about making the config a bit more future-proof and instead of `ConstPlacement` accept what was discussed some time ago:
>
>   QualifierStyle:
>      -   const:     Right
>
>
> and restrict it to `const` for the moment?
>  Maybe renaming to `QualifierPlacement` or something more appropriate.


I'm already feeling there needs to be more fine grained control here in the config... (even if that is a list of MACRO types to ignore) or IgnoreCapitializedIdentifiers, or WarnButDontFix or as we talked about before handling more than just the placement of const, I feel you are correct I'll end up polluting the toplevel config namespace unless I switch to some form of structure in the YAML like we do with BraceWrapping.



================
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:
> 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?




================
Comment at: clang/lib/Format/EastWestConstFixer.cpp:195
+                                FormatToken *Tok) {
+  // We only need to think about streams that begin with const.
+  if (!Tok->is(tok::kw_const)) {
----------------
curdeius wrote:
> Why? What about `unsigned const int`?
@curdeius would you help me understand your expectation here?

 - east: `unsigned int const`
 - west: `const unsigned int`

?


================
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);
----------------
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?




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

https://reviews.llvm.org/D69764