[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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

> With the introduction of `CVQualifierOrder` the need for `CVQualifierAlignment:` doesn't make so much sense
>
>   CVQualifierAlignment: Left
>   CVQualifierOrder: [ "inline", "static", "volatile", "const" ]
>
> I'm leaning toward introducing <type> into the `CVQualifierOrder` allowing for some qualifiers to be Left of the type and some to be Right (this is not dissimilar to what was discussed some time ago, and similar to the concept of the resharper image above)
>
> So we would be able to have:
>
>   CVQualifierOrder: [ "inline", "static", "volatile", "<type>", "const", "restrict" ]
>
> By doing so `inline,static,and volatile` are Left and `const,restrict` are Right and so a single setting of Left and Right isn't applicable.
>
> The whole feature could then be determined ONLY using the `CVQualifierOrder` however it doesn't feel like there is a good way to say "Don't change order" (unless of course I use an empty `CVQualifierOrder` list), but this doesn't feel very off by default enough, I like the explicitness of `Leave`.
>
> My gut feeling is to make `Left` and `Right` options define a predetermined CVQualifierOrder, and introduce `Custom` as a new CVQualifierAlignment setting which would require you to define the Order, this has the advantage of driving a sort of semi standard as to what we mean by `Left` and `Right`
>
> Left would by default define:
>
>   CVQualifierOrder: [ "inline", "static", "volatile", "const", "restrict", "<type>"]    
>
> Right would define:
>
>   CVQualifierOrder: [ "inline", "static", "<type>", "volatile", "const", "restrict" ]
>
> (we might want to discuss what these defaults should be, I'm not a language lawyer!)
>
> I realise "inline" and "static" are to the left in the Right alignment, but I'm not sure them being to the right is even legal (its certainly not common) or what is most likely wanted.
>
> I also think if a `CVQualifierOrder` is defined and the `CVQualifierAlignment` is not `Custom` that should be a configuration error to ensure there isn't any ambiguity
>
> Does anyone have any thoughts on this approach?

I think the general idea (letting the user specify the order of elements relative to one another rather than trying to come up with individual controls for each qualifier), I think that'll be easier for people to configure. I do worry about calling it "qualifier order" though as some of the things are definitely not qualifiers (like `static` or `inline` which are specifiers). A few questions about using an ordered list like this are: 0) should we diagnose typos (e.g., the user writes `inlnie` instead of `inline`)?, 1) should we diagnose duplications (e.g., the user lists `friend` twice)?

Btw, https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Sema/DeclSpec.h#L229 has quite a bit of information about the kinds of qualifiers and specifiers Clang deals with at the parsing stage, in case you were looking for a list of things to care about.

> Oh man you are killing me with good suggestions ;-)....
> So this will be hard with the current implementation because [ [ noreturn ] ] are all separate tokens. However maybe if we COULD merge the tokens into 1 token (like we do in FormatTokenLexer) even it it was just temporarily for this pass we maybe could do that.

I'd like to second the request for attribute support, even if it's follow-up work. Things like address space attributes, GC attributes, `__unaligned`, etc are all type qualifiers (I think we have a list of the attributes which are qualifiers in Type.h), so it's a natural extension. However, attributes in general are somewhat tricky because the attribute style (`__attribute__` vs `[[]]`) can be syntactically location sensitive. e.g., `[[noreturn]] void func()` means something different from `void [[noreturn]]` func()` and we have some attributes that can appertain to a declaration or a type (so the positioning can have silent semantic changes).

> I'm in no rush, I want to let the RFC have a good amount time for people to read and to gather all the feedback, but I'm encouraged enough to think my efforts won't be wasted and I want to address some of @aaron.ballman concerns to extend a olive branch in the hope that I could at least partially gain approval.

Not being in a rush sounds like a great plan! FWIW, I think we're moving steadily towards approval. I mentioned it on the RFC thread, but I'll mention it again here: thank you for starting the RFC process to have the broader discussion!


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

https://reviews.llvm.org/D69764