[llvm-dev] [cfe-dev] [RFC] Adding support for clang-format making further code modifying changes (original) (raw)

Manuel Klimek via llvm-dev llvm-dev at lists.llvm.org
Tue Aug 10 04:36:02 PDT 2021

Fwiw, I think "clang-format can make breaking changes to code when we consider the benefit to be worth it" has been the policy on clang-format for a very long time, so accepting that as the official policy is IMO not a change. If somebody wants to write it down to prevent future revisiting, that seems fine with me.

On Tue, Aug 10, 2021 at 11:32 AM Björn Schäpers via cfe-dev < cfe-dev at lists.llvm.org> wrote:

I'm all in favor of allowing such changes and will help to create and review these changes.

Kind regards, Björn (HazardyKnusperkeks). Am 10.08.2021 um 10:32 schrieb MyDeveloper Day via cfe-dev: > Thanks for the response Sam, > > Here is how I see we mitigate the risk: > > On Mon, Aug 9, 2021 at 11:23 PM Sam McCall <sammccall at google.com_ _> <mailto:sammccall at google.com>> wrote: > > I'm cautiously +1 on const reordering, having previously opposed it and been > convinced. > I think anyone who's worked on a large shared codebase both before and after > clang-format can understand the value here, so I'll focus mostly on the > risks and why I think they're acceptable. > > *Risk: *clang-format will become a grab-bag of features with no clear line - > just anything implemented on top of its pseudo-AST. > Clang-format's brand is low-level formatting details and I think it's > important to preserve this. Const order fits here in users' minds. (So does > brace addition/removal). > > > I doubt we wouldn't continue to apply the same level of scrutiny on the code > reviews and expect them to follow best practices and guidelines, I am expecting > us to still be quite circumspect as to what we'd consider. > > To be honest clang-format I think already runs at quite a high review rejection > rate, people ask for all sorts of things and we do try to push back pretty hard, > landing something can sometimes be pretty torturous to get through review, > I'm not expecting that to change. > > > Risk: The feature will break code and clang-format will no longer be (seen > as) reliable. This can make it harder socially or technically to deploy, and > cause real damage. > I think we need to work hard on mitigating this: > - the feature needs careful design and extra scrutiny, like > security-critical code > - it should be clearly and temporarily marked as experimental, with opt-in > required > - it should be clearly and permanently marked as "makes assumptions about > coding style", with opt-in required. > - bugs need to be thoughtfully addressed > From what I can see MyDeveloperDay is serious about doing all of this. > > > I am, I also think that we shouldn't plough on with individual changes if we see > them as potentially ambiguous, I would rather ignore a change if in doubt, I > don't feel such features need to be 100% catch all (like how sometimes clang > doesn't always tell you about all missing overrides, just as it can rationalize > them), This may require more specific options to ensure we know what an > tok::identifier actually is in order to avoid ambiguities caused by macros (a > little like StatementMacros) > > > Risk: clang-format will be overtaken by the complexity of such features, > which will outweigh the benefits (if few use them), hurting maintenance, > causing bugs etc. > However this isn't different from other optional features. Editing tokens > tends to be done as a separate pass which is relatively easy to isolate > (compared to something like supporting a new language). With complexity > isolated, this is mostly just about how maintainers prioritize their > time/attention, which must be left up to them. > > > To be honest these are likely some of the less invasive features (in comparison > to say adding something like adding Whitesmiths style or C#), as you say the > "Passes" give us an easy mechanisms to handle the "OptIn" without adding "if > (...) everywhere and the passes also tend to be very self contained especially > as the Formatting itself is just a Pass in its own right which is performed later. > > I have no concerns over the maintenance other than ensuring we understand how > new passes actually work, but the compartmentalization feels on a par to > compartmentalization of individual clang-tidy checks. > > > Regarding include-ordering: I think this is a valuable feature if you follow > a coding style that allows it to be correct, and it fits well in > clang-format's brand. However it wasn't clearly labeled to emphasize its > caveats, and in hindsight it shouldn't have been made part of the Google > style without further opt-in required. > > > To be honest as a developer I like the brutality of include-ordering, breaking > my code only tells me it isn't robust enough (likely missing forward > declarations or not including what its using) > > The handling of defaults is always difficult as some people want things and > others don't, (hence the need for the RFC), but I've always been clear this > needs to be "Opt-In" from the start. For the majority of developers I would > expect them to continue to use clang-format as a code formatter and nothing > else, but having a ability to make some (not all) obvious changes could > potentially be a great help to improving code > > For example how many times do you see in LLVM the review comment that says > "elide the braces" for > > if (x) { > return; > } > > I feel this is something that clang-format could be made to easily handle. This > RFC is about gaining a general consensus to let us try. We feel we can add even > more value. > > Anyone who knows me, knows I'm very much pro "clang-format all the things" > > MyDeveloperDay > > > > > _> ________________________ > cfe-dev mailing list > cfe-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev >

cfe-dev mailing list cfe-dev at lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210810/79cefeef/attachment.html>

More information about the llvm-dev mailing list