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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
