It's more the fact that that's /all/ the warning improvement has found so
far. If it was 8 false positives & also found 80 actionable bugs/bad code,
that'd be one thing.

Now, admittedly, maybe it would help find bugs that people usually catch
with a unit test, etc but never make it to checked in code - that's always
harder to evaluate though Google has some infrastructure for it at least.

On Tue, Apr 10, 2018 at 9:07 AM John McCall <> wrote:

> If you have a concrete suggestion of how to suppress this warning for
> user-defined operators just in unit tests, great. I don’t think 8
> easily-suppressed warnings is an unacceptably large false-positive problem,
> though. Most warnings have similar problems, and the standard cannot
> possibly be “must never fire on code where the programmer is actually happy
> with the behavior”.
> John.
> On Tue, Apr 10, 2018 at 17:12 Nico Weber <> wrote:
>> "False positive" means "warning fires but didn't find anything
>> interesting", not "warning fires while being technically correct". So all
>> these instances do count as false positives.
>> clang tries super hard to make sure that every time a warning fires, it
>> is useful for a dev to fix it. If you build with warnings enabled, that
>> should be a rewarding experience. Often, this means dialing back a warning
>> to not warn in cases where it would make sense in theory when in practice
>> the warning doesn't find much compared to the amount of noise it generates.
>> This is why for example clang's -Woverloaded-virtual is usable while gcc's
>> isn't (or wasn't last I checked a while ago) – gcc fires always when it's
>> technically correct to do so, clang only when it actually matters in
>> practice.
>> On Tue, Apr 10, 2018 at 10:21 AM, Roman Lebedev via Phabricator via
>> cfe-commits <> wrote:
>>> lebedev.ri added a comment.
>>> In, @thakis wrote:
>>> > This landing made our clang trunk bots do an evaluation of this
>>> warning :-P It fired 8 times, all false positives, and all from unit tests
>>> testing that operator= works for self-assignment. (
>>> has
>>> the exact details) It looks like the same issue exists in LLVM itself too,
>>> Right, i guess i only built the chrome binary itself, not the tests,
>>> good to know...
>>> > Now tests often need warning suppressions for things like this, and
>>> this in itself doesn't seem horrible. However, this change takes a warning
>>> that was previously 100% noise-free in practice and makes it pretty noisy –
>>> without a big benefit in practice. I get that it's beneficial in theory,
>>> but that's true of many warnings.
>>> >
>>> > Based on how this warning does in practice, I think it might be better
>>> for the static analyzer, which has a lower bar for false positives.
>>> Noisy in the sense that it correctly diagnoses a self-assignment where
>>> one **intended** to have self-assignment.
>>> And unsurprisingly, it happened in the unit-tests, as was expected ^ in
>>> previous comments.
>>> **So far** there are no truly false-positives noise (at least no reports
>>> of it).
>>> We could help workaround that the way i initially suggested, by keeping
>>> this new part of the diag under it's own sub-flag,
>>> and suggest to disable it for tests. But yes, that
>>> Repository:
>>>   rC Clang
>>> _______________________________________________
>>> cfe-commits mailing list
cfe-commits mailing list

Reply via email to