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

Manuel Klimek via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Aug 9 04:00:18 PDT 2021


klimek added a comment.

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

> In D69764#2932929 <https://reviews.llvm.org/D69764#2932929>, @steveire wrote:
>
>> 
>
> @steveire, thanks for your comments, I also agree that a second tool shouldn't be needed, especially as this functionality is off by default (and needs to stay that way). This was my suggestion for a compromise.
>
> Its a tough call, but ultimately no on ever gave this a LGTM so I couldn't proceed anyway. Whilst I think we can cover the other qualifiers with a similar approach I don't want to waste more of my time until the fundamental question has been answered if its ok for clang-format to insert/modify the order of tokens and that is more a collective decision.
>
> It was suggested to me that I write up and RFC on the matter, I'm not a massive fan of those as I don't really see a formal process for handling them (the conversation seems to turn into a series of personal preferences then dwindles and dies without conclusion). But if people think it would solve something I guess I could try.
>
> From my perspective I'm more interested in the views of the major clang-format contributors (@curdeius, @krasimir, @owenpan , @sammccall, @HazardyKnusperkeks  and people like yourselves who have put time and effort into blogging about these tools), ultimately it will be us who have to handle the impact of this (and maybe the #clangd people) and I don't want to inconvenience them or make work for them.
>
> So here is what is needed to land this in my view:
>
> 1. I'd want at least 2 to 3 of the current reviewers to LGTM.
> 2. I'd want 1 person (like yourself) a community contributor to also give their approval (I guess I have that conceptually from you!)
>
> or
>
> 1. @klimek or @djasper to give us an its ok for "clang-format to insert/modify the order of tokens" in a controlled manner decision
> 2. 1 LGTM
>
> Whilst I respect the views of those few who have objected, they don't to my knowledge make significant contributions to clang-format (not in the last 5-6 years I've been following it anyway)
>
> I feel this team owns the decision as we are the ones that look after it, warts and all. We'll always have a situation where some don't agree, thats ok, but I feel there is more agreement that this funcationality is ok rather than not.
>
> Until one of those 2 things happens, we are in limbo.

I think we had a really good, inclusive discussion on this change, so I don't think an RFC would add anything to it.

I think we have consensus on, if we want this, we:

1. want it behind an option that is not enabled in default-configs
2. we want users to understand that this has the potential to introduce bugs

I also think this is a feature that seems useful enough for a large enough group, to provide it in clang-format.
I'd put a lot of weight on @MyDeveloperDay's evaluation, as in the end, he's currently taking the most active role in keeping clang-format alive & well.

One idea for clang-format going forward is: can we make it easier for people to understand what flags can introduce non-whitespace-changes? E.g. have flag name prefixes like semantic-*.

So, I'm generally OK with this.

One minor nit I have is that I'd personally add unit tests to the EastWestConstFixer, as it's quite a large class, but given that I'll duck out after this comment, I'll not insist :)


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

https://reviews.llvm.org/D69764



More information about the cfe-commits mailing list