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

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>
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210810/631677a2/attachment.html>

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
>

On Tue, 10 Aug 2021 at 09:32, MyDeveloper Day via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> 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"
>
I agree with most of the sentiment in this thread, too. clang-format can be
dangerous but it can also be so much more.

I have been using clang-format on editors, pre-commit checks and ninja
targets for a quite a while and I think it's a fundamental development
tool. With it, style comments stop being a personal conversation and become
tooling.

I would like to see a world where there are no more long styling documents
with personal angst in all projects, just a clang-format config file.

I'm in favour of increasing the potency of clang-format. Quick checks can
be done on save, more expensive ones as ninja targets and even more
expensive ones as CI.

To me, the discussion of defaults and perils must be clear to all users and
documentation may not be the best way: I've never read the clang-format
documentation.

Another problem of clang-format is that there are so many options but not
many people know enough of them.

Yet another problem of clang-format is that its behaviour is different for
the same options over different versions. We're stuck on clang-format-9,
not because it's "the best" but because re-formatting the whole
tree every
time a new tool comes out is silly.

So a good solution to all of these problems is to generate a default
configuration file with all the options and comments on each one of them,
including "this changes non-whitespace code", "this is quite
slow", "if
this breaks code, here's how to fix", etc, and setting all of them to
the
default value.

We also nee to be able to update the config file with new options without
destroying the old (often modified) ones, and make sure new versions only
do new things if the config is different.

I know, configuration management and backwards compatibility aren't
trivial, but managing clang-format over time isn't either, and it should.
Well, it should, if it wants to be a catch-all tool to do all the things.

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