[llvm-dev] [cfe-dev] [RFC] Adding support for clang-format making further code modifying changes
Aaron Ballman via llvm-dev
llvm-dev at lists.llvm.org
Tue Aug 10 05:35:33 PDT 2021
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
More information about the llvm-dev
mailing list