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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 2 inline comments as done.
MyDeveloperDay added inline comments.


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3233
+
+   ``QualifierAlignment`` COULD lead to incorrect code generation.
+
----------------
HazardyKnusperkeks wrote:
> simon.giesecke wrote:
> > This is pretty unclear, for a number of reasons:
> > * First, this probably only refers to a setting other than `QAS_Leave`?
> > * Second, "lead to incorrect code generation" seems to skip a step. In the first place, this seems to imply that a setting other than `QAS_Leave` might change the semantics of the source code.
> > * Third, it's not clear to me when this would happen, and how likely that is. Does this mean "Non-default settings are experimental, and you shouldn't use this in production?" or rather "Under rare circumstances that are unlikely to happen in real code, a non-default setting might change semantics." At the minimum, could this give some example(s) when this happens?
> * Yes.
> * Yes, it might change the semantics, that was the content of a huge discussion here in the review and on the mailing list. Consensus was found that non whitespace changes are acceptable with a big warning and off by default.
> * The latter, if we would have an example at hand in which cases it wouldn't work, we would fix that and not give it as an example. So to the best of our knowledge it does not break anything.
> 
> The warning text however might be improved.
Agreed its tough to give a meaningful example that actually currently breaks code, but the example I was given was

```
#define INTPTR int *

const INTPTR a;
```

In this sense moving const changes this from

```
const int * a;
```
to 

```
int * const a;
```

For this we overcome this by NOT supporting "UPPERCASE" identifiers incase they are macros, we have plans to add support for identifying "type identifiers"

I guess if someone does

```
#define intptr int *
```

Then this could go still go wrong, however I like to think that these examples are on the "edge" of good code. (should we pander to what could be considered bad style in the first place?)

The warning was a concession to those who felt its should be highlighted as an option that could change behaviour (like SortIncludes was famously identified),  and as @HazardyKnusperkeks  say, we are looking for people to tell us where this breaks so we can try and fix it. (macros are already an issue for clang-format the solution for which is clang-format off/on)

I personally feel the no need to elaborate further on the warning at this time, but I'm happy to support someone if they feel they want to extend it. 

If you are concerned my advice is don't use this option. But clang-format can be used in an advisor capacity (with -n or --dry-run) and this can be used to notify cases of incorrect qualifier ordering without actually using it to change the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69764