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

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>

On Tue, 10 Aug 2021 at 15:55, Manuel Klimek via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> 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.
>
I think there's more to that than what was created by the people who
created it.

After being used extensively by people around the world, clang-format has
perhaps grown to something that the original team did not envision.

However, as a public clang tool, and as part of the LLVM "family",
it's no
longer "a tool used by the team that created it", and not even "a
tool used
by LLVM developers". It is truly a public tool maintained by the LLVM
community.

As such, I think we shouldn't restrict the design goals to the original
design, or to what a small sub-group uses it for. Clang-format is an
official tool like other visible stuff and should have the same community
oversight as everything else.

There is a high demand for a formatting tool that can do so much more than
the original design and people have been using it, albeit precariously, for
that purpose already.

So I believe the original policy that "breaking changes are a benefit"
can
still be valid in a number of places, but it does not (and should not) need
to be the only valid case. Definitely not the default, either.

I think we need to take a step back and ask users what they expect, rather
than push forward a policy that people never really wanted but coped with
it anyway because there's nothing better.

FWIW, borrowing suggestions from others in this thread, I propose we change
the policy to:
 * Breaking changes off by default, override behaviour with configuration
files
 * All behaviour must be controlled by a configuration flag with options
explicit on doc/config
 * Make explicit some options are breaking (comment/naming)
 * Backwards compatibility is pursued, but can be broken in case of clear
bugs

An easy way to manage the configuration in this case is to have a dump
function (`clang-format --dump --config foo.cfg`) that reads a config file
and dumps all missing parameters and their current values.

Inline comments would be nice but make configuration management hard, so
there could be a webpage that describes all options and the URL is dumped
on the config file.

cheers,
--renato
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210810/d114e4cb/attachment.html>