[PATCH] D152473: [clang-format] Add test case for issue 63170

Wed Jun 14 14:44:19 PDT 2023

owenpan added a comment.

In D152473#4410030 <https://reviews.llvm.org/D152473#4410030>, @paulkirth wrote:

> In D152473#4409975 <https://reviews.llvm.org/D152473#4409975>, @MyDeveloperDay wrote:
>
>> In D152473#4409146 <https://reviews.llvm.org/D152473#4409146>, @paulkirth wrote:
>>
>>> The point of this patch was to show a regression. I'm not trying to fix anything per se. I'd like https://reviews.llvm.org/D151954  and anything that depends on it reverted, since it breaks `clang-format`.
>>
>> I think its respectful to wait a little for the code owners to give you a LG unless the patch is breaking the unit tests.
>
> So first, no one here is trying to be disrespectful, we're all here to make the compiler and surrounding tools better for everyone. That said, it's also normally respectful to revert changes when others report that your patch has a serious regression.  https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy
>
> I'm not sure how common this is, but in this case we found a test case that was missing from the unit tests, otherwise the patches that were reverted would never have landed. So, while I understand your point, reverting here is in line w/ the communities policies. If there is a different interpretation I should have on the policy, I'm happy to hear it, but as I read it, our response here is in line w/ our typical developer expectations. My apologies if any of this comes off as rude (being nuanced in text is hard) but I'm trying to be very clear about my interpretation and rationale.

Please note the following from the patch reversion policy you linked above:

> If you are asked to revert by another contributor, please revert and discuss the merits of the request offline (unless doing so would further destabilize tip of tree).

and

> Reverts should be reasonably timely. ... If you are unsure, we encourage you to reply to the commit thread, give the author a bit to respond, and then proceed with the revert if the author doesn’t seem to be actively responding.

The revert 94e75469597f <https://reviews.llvm.org/rG94e75469597f197f9c4b45baa6c8a576c78dbd02> was wrong as it destabilized the tip of the tree by breaking clang-format unit tests. (See D151954#4404731 <https://reviews.llvm.org/D151954#4404731>.) Then another revert (i.e. this patch) accompanied with a test case irrelevant to D152305 <https://reviews.llvm.org/D152305> that it was reverting was rushed in in order to "fix" the unit tests that 94e75469597f <https://reviews.llvm.org/rG94e75469597f197f9c4b45baa6c8a576c78dbd02> had broken. Also, the regression was triggered by trailing whitespaces in your files. Although reverting D151954 <https://reviews.llvm.org/D151954> solves the regression for you, it would leave other users in a limb as a few patches prior to and depending on it might have to be reverted as well. Maybe you could have temporarily worked around the regression (e.g. by removing the trailing spaces) while we were investigating the issue?

> As a last point we've had our CI broken by this since the initial patch landed, and it's been several days since we first reported the issue. We're certainly not trying to be discourteous w/ our actions, but it I also don't think it's reasonable to expect that when community members report a serious regression, file an issue, and provide a reproducer that they be expected to wait days for a revert or forward fix.

Please review the timestamps. You first reported the issue in https://reviews.llvm.org/D151954#4403921, and the revert 94e75469597f <https://reviews.llvm.org/rG94e75469597f197f9c4b45baa6c8a576c78dbd02> occurred less than 6 hours later.

Anyway, I've relanded D152305 <https://reviews.llvm.org/D152305> using exactly the same diff and am working on a better solution than D151954 <https://reviews.llvm.org/D151954>. I will add you to the review list to make sure that it won't break your CI.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152473/new/

https://reviews.llvm.org/D152473