llvm dev - Aug 2021 - [cfe-dev] [RFC] Adding support for clang-format making further code modifying changes

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>

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!).

~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