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

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D69764#2056104 <https://reviews.llvm.org/D69764#2056104>, @rsmith wrote:

> I'm uncomfortable about `clang-format` performing this transformation at all. Generally, clang-format only makes changes that are guaranteed to preserve the meaning of the source program, and does not make changes that are only heuristically likely to be semantics-preserving. But this transformation is not behavior-preserving in a variety of cases, such as:
>
>   #define INTPTR int *
>   const INTPTR a;
>   // INTPTR const a; means something else
>


At least in my case, our codebase doesn't have code like that. I wonder how common it is.

> I also don't think this is something that a user would *want* in `clang-format`: changing the location of `const`s is something that would likely be done rarely (probably only once) when migrating to a different coding style, rather than being done on the fly after each edit to a source file.

I don't think this is true.

All of the arguments in favor of `clang-format` existing apply here.

- Developers get it wrong. They put the `{` or the `*` in the "wrong" place according to the style guide, and they put the `const` in the "wrong" place according to the style guide.
- `clang-format` is much faster than `clang-tidy` and it is reasonable to have text editors run it on every "Save" and on all files in a repo on every git commit etc.
- The above means that the cost of developers getting it wrong is minimized or eliminated
- The above means that developers don't have to pay attention to `*` placement or `const` placement while writing code, but can (in theory at least) focus on what they're trying to convey.
- It seems that more tools understand `clang-format` than `clang-tidy` (eg text editors with support for it)

> Fundamentally, I don't think this transformation is simply reformatting, and I don't think it can be done sufficiently-correctly with only a largely-context-free, heuristic-driven C++ parser.

I agree that this is a less simple transformation than existing `clang-format` functionality.

I can't evaluate whether your macro example (or other examples you could come up with) mean that it can't be done sufficiently-correctly, but I doubt I would come to the same conclusion. I would probably base that on whether I could find real-world code which it breaks, and how common the breaking-patterns (the macro in your example) actually are in other code.

> As such, I think this belongs in `clang-tidy`, not in `clang-format`.

Given the speed difference and the developer convenience, I think this would be very unfortunate. I've already tried to write this conversion as a `clang-tidy` check. However, my implementation has far more bugs than this `clang-format` implementation, and it does not handle as many cases. You can see that many `clang-tidy` checks exclude types of code such as "all templates" from transformation to dampen the complexity of the check implementation.

Besides, a clang-tidy implementation would run into the same problem with your macro example, wouldn't it? Often the solution to that kind of problem in `clang-tidy` checks is to simply not transform code in macros. Would an option in clang-format to not transform around macro code be an more acceptable solution?


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

https://reviews.llvm.org/D69764