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

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

*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.
*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.

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.

Cheers, Sam
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210810/4e2556c6/attachment.html>

On Mon, Aug 9, 2021 at 3:23 PM Sam McCall via cfe-dev <
cfe-dev at lists.llvm.org> 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).
>
> *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.
>
> *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.
>
> 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.
>
Thanks for the write up! & for the record I'm pretty comfortable with
what
Sam's said here. (not that I've got strong weight in clang-format
development, but to make sure my other comments on the thread aren't
treated as standing criticisms/concerns)

- Dave
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210809/5a3a80d0/attachment.html>

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>