<div dir="ltr"><div dir="ltr">On Wed, Feb 20, 2019 at 10:29 AM Michael Kruse <<a href="mailto:llvmdev@meinersbur.de">llvmdev@meinersbur.de</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">maybe another interesting case is when there is disagreement over<br>
whether there is a regression.<br>
<br>
For instance, one of my patches made clang emit an additional warning<br>
when compiling a popular project. This was not intentional by my<br>
patch, but due to an inconsistent implementation of the warning in<br>
clang. However, the warning was legitimate. I reverted the patch (it<br>
had another problem), but before I recommitted it, I put up a patch<br>
for review that fixed the inconsistent implementation such that the<br>
warning is always emitted.<br>
<br>
My question here: Should the patch be reverted even if it did not have<br>
the other problem?<br></blockquote><div><br></div><div>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.</div><div><br></div><div>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.</div></div></div>