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

[Aaron Ballman via llvm-dev]

On Mon, Aug 30, 2021 at 7:08 PM David Blaikie <dblaikie at gmail.com> wrote:
> On Sun, Aug 29, 2021 at 6:46 AM Aaron Ballman <aaron at aaronballman.com> wrote:
>>
>> 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.
>
>
> Yeah, this is the vague/uncertain aspects I'm a bit concerned with: If someone comes along with a test case demonstrating a formatting rules can break certain code and folks classify it as effectively "will not fix because it's not worth it/diminishing returns" we now have to change the rule to be off-by-default (where it was possibly on-by-default before). But then if someone decides to fix that bug, because they have a use-case that makes it worthwhile for them to fix it - now we might flip it back to on-by-default?

Ah, I see where your concerns come in now. Thank you! I was
envisioning that once we moved an option from on-by-default to
off-by-default, it would most likely never move again. My assumption
is: if the breaking change could be fixed, it would have been fixed or
confirmed as a bug rather than switching to off-by-default. So we
could have some on-by-default checks that DO change code for a while,
but there's a grace period of sorts where clang-format can work on the
fix to the bug. If the bug turns out to not be fixable or no one shows
an interest in fixing it by the time there's a second reporter, then
we switch it to off-by-default at that point and it stays there,
barring extenuating circumstances. Because of the inexact nature of
how clang-format works, I guess my feeling is that once a
transformation has been demonstrated to break code in a way that's
difficult enough we needed to turn the feature off by default, it's
reasonable to assume it's going to break other code we've not
considered yet and so the bar is raised much higher to try to switch
it back to on-by-default, so these flip flops should be vanishingly
rare.

> If that's unlikely - if most of the time the answers are clear, that the breakage is /super/ expensive to avoid such that no one's likely to ever revisit the decision/have a different cost/benefit tradeoff - and the breakages are obvious/easy to demonstrate (like I said, if there's common types of breakage and so simple breakage patterns that can be used in most cases to demonstrate breakage, etc) then we might avoid these sort of flip/flop cases most of the time.

Yeah, I would hope to avoid flip flopping like that.

~Aaron