aaron.ballman added a comment.

In D96223#2788681 <https://reviews.llvm.org/D96223#2788681>, @lebedev.ri wrote:

> Reverted in be6b9e8ae71768d2e09ec14619ca4ecfdef553fa 
> <https://reviews.llvm.org/rGbe6b9e8ae71768d2e09ec14619ca4ecfdef553fa> - 
> https://godbolt.org/z/3zdqvbfxj
> While there, please ensure that other such simplifications don't have similar 
> lingering effects.

Thanks for catching the issue!

FWIW, I think that reverting something several months after it lands is a bit 
disruptive and the test case causing the revert feels arbitrary without further 
context that should have been explicitly mentioned in the reverting commit 
message. The context is that this change broke a test that should have caught 
the failure: 
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/checkers/misc-static-assert.cpp#L86-L87
 which led to this bug report: https://bugs.llvm.org/show_bug.cgi?id=50532. I'm 
still not convinced the correct action was to revert a change with no 
discussion that already was shipped in a release, however. Next time, I think 
it'd be a bit friendlier to mention the regression on the patch to see if the 
author can do a quick fix -- if no one noticed it for almost four months, it's 
hard to argue it's a critical issue that needs an immediate revert. (The 
reversion mildly worries me because this change already shipped as part of 
Clang 12 and larger reversions make for harder git blame logic to follow along 
with, both of which can lead to potential confusion for folks.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96223

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to