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

Sun Aug 29 06:46:13 PDT 2021

On Sat, Aug 28, 2021 at 3:40 PM David Blaikie <dblaikie at gmail.com> wrote:
>
> On Sat, Aug 28, 2021 at 5:52 AM Aaron Ballman <aaron at aaronballman.com> wrote:
>>
>> On Sat, Aug 28, 2021 at 3:48 AM David Blaikie <dblaikie at gmail.com> wrote:
>> >
>> > +1 to what Manuel's said here.
>> >
>> > One slight change I'd suggest is changing the term "breaking changes" to "non-whitespace changes", perhaps? (they aren't necessarily breaking anything) At least I assume that's the intent, but I might be wrong in which case I'd love to better understand what's being proposed.
>>
>> To me, the crux of my concern isn't nonwhitespace changes, but changes
>> that can make code which used to compile no longer do so. It just so
>> happens that nonwhitespace changes are where that risk is highest, but
>> whitespace changes can impact syntactic validity (such as reformatting
>> preprocessor directives which terminate with an end of line) and
>> nonwhitespace changes can (in theory) be written to not break code
>> (such as inserting parentheses in expressions such that they match the
>> current order of operations used by the expression). So I prefer
>> "breaking changes" because it captures the concern better, but I'd
>> reword it to "potentially breaking changes" to improve clarity. It's
>> not that we expect the option to break significant code (that'd be
>> bad), it's that we anticipate that it *could* break code and that's
>> why it's treated with greater caution.
>
>
> The language here seems a bit too vague/guarded for my comfort level. Is the expectation then that someone must demonstrate a specific breakage situation (however rare/unlikely?) to justify classifying the formatting feature into this special off-by-default/risky group?

Sort of? My thinking is: if someone comes up with a test case that the
clang-format developers do not consider to be a bug (and thus don't
intend to "fix" the behavior), then we absolutely should document it
as described. (Also, I think it should be an explicit test case
showing the behavior with a comment about it being expected behavior
-- that helps with communication if someone files a bug about it.) If
no one can come up with a test case that would break, I'd be fine if
we still classified it as a potentially breaking change based on
whitespace vs not whitespace or some other metric, but my bigger point
is that if someone can demonstrate a test case that break that isn't
sufficiently compelling to change the tool to handle better, that
definitely meets the bar for being opt-in.

> Perhaps we'll end up with easy idioms/obvious proofs that apply to whole classes (perhaps, for instance, there's an easy/obvious/reusable proof that applies to most cases of token reordering similar to what Arthur showed?) of changes & that'll keep the burden of proof fairly low?

That seems plausible.

> In any case, I see the language is intentional, and if the folks working on this are comfortable with it, I'll leave it to you folks - thanks for explaining!

Any time, thanks for the discussion!

~Aaron

>
> - Dave
>
>>
>>
>> ~Aaron
>>
>> >
>> > On Fri, Aug 27, 2021 at 7:03 AM Manuel Klimek via cfe-dev <cfe-dev at lists.llvm.org> wrote:
>> >>
>> >> On Fri, Aug 27, 2021 at 1:43 PM Aaron Ballman via cfe-dev <cfe-dev at lists.llvm.org> wrote:
>> >>>
>> >>> Thanks to everyone who participated in the discussion, and especially
>> >>> to MyDeveloperDay for getting this started. Now that discussion on
>> >>> this RFC seems to have died down, I think it's worth circling back to
>> >>> see if we have consensus to move forward. From reading the threads, I
>> >>> think Renato summarized the consensus position nicely:
>> >>
>> >>
>> >> I agree with the following items:
>> >>
>> >>>
>> >>>  * 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)
>> >>
>> >>
>> >> I disagree with this one:
>> >>
>> >>>
>> >>>  * Backwards compatibility is pursued, but can be broken in case of clear bugs
>> >>
>> >>
>> >> Which I think is also tangential to the current RFC, and IMO should be treated in a separate discussion if necessary (this is a fundamental problem with how clang-format is designed).
>> >>
>> >>>
>> >>> Unless there is strong opposition to this, then I'd say MyDeveloperDay
>> >>> has an answer to their RFC. Any disagreement?
>> >>>
>> >>> ~Aaron
>> >>>
>> >>> On Wed, Aug 11, 2021 at 4:37 AM Mehdi AMINI via cfe-dev
>> >>> <cfe-dev at lists.llvm.org> wrote:
>> >>> >
>> >>> >
>> >>> >
>> >>> > On Tue, Aug 10, 2021 at 5:24 PM Renato Golin via cfe-dev <cfe-dev at lists.llvm.org> wrote:
>> >>> >>
>> >>> >> 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
>> >>> >
>> >>> >
>> >>> > FWIW, this seems very reasonable to me.
>> >>> > (and it is also what I understood MyDeveloperDay's original proposal to be).
>> >>> >
>> >>> > Cheers,
>> >>> >
>> >>> > --
>> >>> > Mehdi
>> >>> >
>> >>> >
>> >>> >>
>> >>> >>
>> >>> >> 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
>> >>> >> _______________________________________________
>> >>> >> 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
>> >>> _______________________________________________
>> >>> 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