[llvm-dev] Clarification on expectations of buildbot email notifications

[Reid Kleckner via llvm-dev]

On Wed, Feb 20, 2019 at 10:29 AM Michael Kruse <llvmdev at 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20190220/49146c25/attachment.html>