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

Wed May 27 01:02:45 PDT 2020

MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added a comment.

@rsmith, firstly let me thank you for taking the time to review this, I didn't realize i'd invoke such a reaction that the big guns would start showing up.. fundamentally I agree with you, but let me defend my thesis to help explain my reasons why I still feel this has value.

clang-format is in my view no longer just a code formatter used for transforming whitespace, This changed when we added the sorting of headers and the adding of namespace comments, but fundamentally clang-format has always worked on what is basically the tooling::Replacements interface, extending this to alter code is a likely natural progression.

Whilst I understand your point about clang-tidy, alas I agree with @steveire its not a viable solution in my view and I don't think it would catch the case you suggest either. Its also somewhat slower and needs so much more information, In very large million line projects its akin to a nightly build and not a sub second sanity check. (In my view clang-tidy is  not the right tool for this job).

I do understand they will be failure scenarios, (and maybe there is something we can do about those), but...

Clang-format has always been able to break your code especially in extraneous circumstances where people use macros (the `__LINE__` example here made me smile!)
https://stackoverflow.com/questions/37927553/can-clang-format-break-my-code

And a recent well publicized issue highlighted that even if we change something that seems semantically the same, it may not be! But that isn't a reason to not use clang-format in my view, its just a reason to be more careful.
https://travisdowns.github.io/blog/2019/11/19/toupper.html

But for me this change is NOT about those corner cases because clang-format is not bug free, it does make mistakes, as such it WILL break code, and even if it doesn't break it semantically, it will from time to time it makes it look like it threw up on your editor (or its drunk https://twitter.com/Manu343726/status/1124686897819934720)  , but we have a workaround for those cases, `//clang-format off` for anything we can't fix and http://bugs.llvm.org for those we can.

You are correct in that this change is most useful during the conversion of a project, and as such its value might initially seem limited, this is why its an `opt in` option, `Leave/East/West` with the Default always being to "Leave as is". However anyone bringing in a new clang-format option or clang-format completely always needs to be careful when reviewing the changes it suggests, there will be corner cases (especially around macros) as there are with other clang-format options which can break the code.

My expectation is that its most useful usage is with the command line argument --const-placement=west/east being added to clang-format being run in dry-run mode as part of premerge type checking and used to reject code review which fails that..  I highlighted this once before in this trail run (in polly), where code has slipped in as east const into LLVM and missed during review.

  clang-format -n --const-placement=west *.cpp

  ScopBuilder.cpp:74:9: warning: code should be clang-formatted [-Wclang-format-violations]
  static int const MaxDimensionsInAccessRange = 9;
          ^
  ScopInfo.cpp:113:2: warning: code should be clang-formatted [-Wclang-format-violations]
  int const polly::MaxDisjunctsInDomain = 20;
  ....
   ^

But for someone like me managing a multi million line C++ code base with developers in the 4 corners of the globe with a constant turnover of new staff, this is the ultimate value.. I no longer have to persuade some new senior engineer who thinks they have been hired for their coding prowess who insists their bracketing and coding style is the most beautiful in the world when the rest of us know its butt ugly.!  Quite simply they cannot get their code committed unless it conforms to our style guide, not because I say so, but because the faceless tool of clang-format says "No!", there is value right there in reducing the arguments and stress alone.

LLVM's own pre-merge checking is reducing our need to keep tell people to clang-format in code review, and this change is about building that ability to reject code before it gets committed, to reduce the code review burden.

And Finally this change is about trying to banish the inane conversations about what "const" is "best const" in the same way as we have somewhat done with white-space and tabs.  I just don't think we should waste our time talking about it, just clang-format it and "you do you", this is what I want out of this because I'm fed of up seeing brilliant minds (http://slashslash.info/2018/02/a-foolish-consistency/)  argue about it when a 99.9% solution is a couple of 100 line patch.

This is the defense of my work and why I and I believe others think there is value here.

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

https://reviews.llvm.org/D69764