[cfe-dev] [RFC] Adding support for clang-format making further code modifying changes
Manuel Klimek via cfe-dev
cfe-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/cfe-dev/attachments/20210810/79cefeef/attachment.html>
More information about the cfe-dev
mailing list