simon.giesecke added inline comments.
================ Comment at: clang/docs/ClangFormatStyleOptions.rst:3233 + + ``QualifierAlignment`` COULD lead to incorrect code generation. + ---------------- MyDeveloperDay wrote: > 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. Thanks a lot for the explanation. Sorry I hadn't read through the entire review discussion. I saw the comment in the generated documentation and then dug up this patch that introduced it. I see how macros might break this (I think I ran into a similar problem when trying to fix the const alignment manually using some `sed` machinery, though luckily it broke the build and didn't end up in bad code generation). It would be good to understand if there are any cases not involving macros whose semantics are modified. Identifying macros via the naming convention can't be reliable. Even if one did that consistently in their own code base, chances are high that library headers don't follow the same conventions. And yes, probably cases where a qualifier is added via a macro is not good style anyway. I replaced that by `std::add_const_t` in the mentioned case. But the use cases of clang-format are probably not confined to code written in a good style. I think it's good to extend the warning a bit, otherwise anyone reading it would need to dig up this patch and read through the review discussion to judge it. I'll leave a comment on D110801 as well. > If you are concerned my advice is don't use this option. Well, I think if it works "reliably", it's very useful. I was searching for a way to harmonize the const/qualifier alignment style, and I first thought that would be clang-tidy land, and then found this clang-format patch. And I guess, maybe that's exactly the gap that leads to this problem: In clang-tidy, there would be the extra semantic information (we know what's a macro or type etc.) that would allow to prevent any semantic changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/ https://reviews.llvm.org/D69764 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
