[llvm-dev] [cfe-dev] [RFC] Adding support for clang-format making further code modifying changes
Manuel Klimek via llvm-dev
llvm-dev at lists.llvm.org
Tue Aug 10 07:54:40 PDT 2021
On Tue, Aug 10, 2021 at 2:35 PM Aaron Ballman <aaron at aaronballman.com>
wrote:
> On Tue, Aug 10, 2021 at 7:37 AM Manuel Klimek via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> >
> > 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.
>
> FWIW, I've never had the impression that this was the status quo for
> the tool. I've looked through the mailing list archives and some old
> reviews and I don't see any supporting evidence one way or the other
> (obviously I could have missed something though!).
>
I agree that that was probably not obvious or stated very clearly anywhere
- clang-format for years was basically developed by a small team without
much interest in input by the rest of the world, for a very specific use
case.
At the time we did not do a great job at spreading our thoughts with the
world, as a lot of discussions were in person.
> ~Aaron
>
> >
> > 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
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210810/5b6cb78e/attachment-0001.html>
More information about the llvm-dev
mailing list