sammccall added a comment. Random thoughts from the peanut gallery:
- `clang-format` *is* the place I'd expect/want this feature, as a user. It's way more similar to `int *x` -> `int* x` than it is to e.g. typical clang-tidy rewrites. My only concern is whether we can give it the safety users expect. - `clang-tidy` has a much higher barrier to entry: slow runtime, complex configuration, build-system integration, more false-positives in common scenarios. e.g. Google has a pretty sophisticated CI system that integrates both: clang-format is typically blocking and clang-tidy is typically not. It would be less useful in clang-tidy. - AIUI there are two identified sources of unsafety. They're real costs for sure, we should mitigate them as much as possible. - bugs: it seems reasonably likely these can be identified and fixed, based on what we've seen in this patch. I'd be more worried about maintenance if this was a drive-by contribution, but it's the opposite of that. - macros: there is some precedent for clang-format being bad or even unsafe in the presence of unknown macros. We could include a list of "don't touch qualifiers around" macro names in config. @klimek has a general-purpose mechanism nearly ready where you can provide actual macro definitions in config. It's also unclear if there's a common pattern where we'd see this. - typedefs don't introduce any such issues, right? - one general mitigation is not including this in any default styles. We could go further and not support setting it in a style file (only as a flag) for clang-format 11, to shake out bugs. - I think `const` is by far the most important qualifier to handle: volatile is rare, others cause less semantic confusion and are generally less controversial. IMO it's fine for this patch to only handle const as long as we know how the configuration could be expanded in future. Shipping is a feature. - To bikeshed the configuration once more: what about `QualifierOrder: [Const, Type]`? This seems fairly self-explanatory, sidesteps "left" vs "west", and expands naturally to `[Static, Const, Type]` in future. It requires some nontrivial validation though. 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
