(original) (raw)
On Wed, Feb 20, 2019 at 10:29 AM Michael Kruse <llvmdev@meinersbur.de> wrote:
maybe another interesting case is when there is disagreement over
whether there is a regression.
For instance, one of my patches made clang emit an additional warning
when compiling a popular project. This was not intentional by my
patch, but due to an inconsistent implementation of the warning in
clang. However, the warning was legitimate. I reverted the patch (it
had another problem), but before I recommitted it, I put up a patch
for review that fixed the inconsistent implementation such that the
warning is always emitted.
My question here: Should the patch be reverted even if it did not have
the other problem?
I would say not necessarily. If a new warning is added that fires on a popular project and the warning is working as intended, that may be a signal that the warning shouldn't be on by default (or shouldn't be part of -Wall). We obviously need to allow ourselves the freedom to add new warnings over time. Just because a project uses "-Werror -Wall" doesn't mean that their code will compile cleanly with new compilers. However, if the warning really is low value, then we may want to remove it. If someone wants to revert a new warning because it is too noisy or has false positives, they need to actively engage the patch author to support their position.
Elaborating on your question of what constitutes a regression in general, I would say that everything is case-by-case, but we have some clear cut ones, like miscompiles or new compiler crashes. People often misdiagnose UB as a miscompile, so to revert for a miscompile, you need some evidence that the code doesn't exercise UB. Reverting for a performance regression would need to come with a strong rationale to motivate why the slowdown outweighs the supposed benefits of the patch in question.