<div dir="ltr">Any good ideas on how to evaluate it, then? (in addition to/other than something like Google where we can track the diagnostic for - a few months?)</div><br><div class="gmail_quote"><div dir="ltr">On Tue, Apr 10, 2018 at 9:34 AM John McCall <<a href="mailto:rjmccall@gmail.com">rjmccall@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Yeah, -Wself-assign in general is about catching a class of live-coding errors which is pretty unlikely to ever make it into committed code, since the code would need to be dead/redundant for the mistake to not be noticed. So it’s specifically a rather poor match for the static analyzer, and I would not expect it to catch much of anything on production code. <br><br>John.<br><div class="gmail_quote"><div dir="ltr">On Tue, Apr 10, 2018 at 18:27 David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, Apr 10, 2018 at 9:25 AM Roman Lebedev <<a href="mailto:lebedev.ri@gmail.com" target="_blank">lebedev.ri@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Because *so far* it has been running on the existing code, which i guess<br>
was already pretty warning free, and was run through the static analyzer, </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
which obviously should catch such issues.<br></blockquote></div></div><div dir="ltr"><div class="gmail_quote"><div><br>Existing code this has been run over isn't necessarily run against the static analyzer already. (this has been run against a subset of (maybe all of, I forget) google, which isn't static analyzer clean by any stretch (I'm not even sure if the LLVM codebase itself is static analyzer clean)).<br> </div></div></div><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Do you always use [clang] static analyzer, each time you build?<br>
Great! It is indeed very good to do so. But does everyone?<br>
<br>
This particular issue is easily detectable without full static analysis,<br>
and as it is seen from the differential, was very straight-forward to implement<br>
as a simple extension of pre-existing -Wself-assign.<br>
<br>
So if this is really truly unacceptable false-positive rate, ok, sure,<br>
file a RC bug,<br>
and i will split it so that it can be simply disabled for *tests*.<br>
<br>
That being said, i understand the reasons behind "keep clang diagnostics<br>
false-positive free". I don't really want to change that.<br>
<br>
On Tue, Apr 10, 2018 at 7:13 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
> It's more the fact that that's /all/ the warning improvement has found so<br>
> far. If it was 8 false positives & also found 80 actionable bugs/bad code,<br>
> that'd be one thing.<br>
><br>
> Now, admittedly, maybe it would help find bugs that people usually catch<br>
> with a unit test, etc but never make it to checked in code - that's always<br>
> harder to evaluate though Google has some infrastructure for it at least.<br>
><br>
> On Tue, Apr 10, 2018 at 9:07 AM John McCall <<a href="mailto:rjmccall@gmail.com" target="_blank">rjmccall@gmail.com</a>> wrote:<br>
>><br>
>> If you have a concrete suggestion of how to suppress this warning for<br>
>> user-defined operators just in unit tests, great. I don’t think 8<br>
>> easily-suppressed warnings is an unacceptably large false-positive problem,<br>
>> though. Most warnings have similar problems, and the standard cannot<br>
>> possibly be “must never fire on code where the programmer is actually happy<br>
>> with the behavior”.<br>
>><br>
>> John.<br>
>> On Tue, Apr 10, 2018 at 17:12 Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>> wrote:<br>
>>><br>
>>> "False positive" means "warning fires but didn't find anything<br>
>>> interesting", not "warning fires while being technically correct". So all<br>
>>> these instances do count as false positives.<br>
>>><br>
>>> clang tries super hard to make sure that every time a warning fires, it<br>
>>> is useful for a dev to fix it. If you build with warnings enabled, that<br>
>>> should be a rewarding experience. Often, this means dialing back a warning<br>
>>> to not warn in cases where it would make sense in theory when in practice<br>
>>> the warning doesn't find much compared to the amount of noise it generates.<br>
>>> This is why for example clang's -Woverloaded-virtual is usable while gcc's<br>
>>> isn't (or wasn't last I checked a while ago) – gcc fires always when it's<br>
>>> technically correct to do so, clang only when it actually matters in<br>
>>> practice.<br>
>>><br>
>>> On Tue, Apr 10, 2018 at 10:21 AM, Roman Lebedev via Phabricator via<br>
>>> cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
>>>><br>
>>>> lebedev.ri added a comment.<br>
>>>><br>
>>>> In <a href="https://reviews.llvm.org/D44883#1063003" rel="noreferrer" target="_blank">https://reviews.llvm.org/D44883#1063003</a>, @thakis wrote:<br>
>>>><br>
>>>> > This landing made our clang trunk bots do an evaluation of this<br>
>>>> > warning :-P It fired 8 times, all false positives, and all from unit tests<br>
>>>> > testing that operator= works for self-assignment.<br>
>>>> > (<a href="https://chromium-review.googlesource.com/c/chromium/src/+/1000856" rel="noreferrer" target="_blank">https://chromium-review.googlesource.com/c/chromium/src/+/1000856</a> has the<br>
>>>> > exact details) It looks like the same issue exists in LLVM itself too,<br>
>>>> > <a href="https://reviews.llvm.org/D45082" rel="noreferrer" target="_blank">https://reviews.llvm.org/D45082</a><br>
>>>><br>
>>>><br>
>>>> Right, i guess i only built the chrome binary itself, not the tests,<br>
>>>> good to know...<br>
>>>><br>
>>>> > Now tests often need warning suppressions for things like this, and<br>
>>>> > this in itself doesn't seem horrible. However, this change takes a warning<br>
>>>> > that was previously 100% noise-free in practice and makes it pretty noisy –<br>
>>>> > without a big benefit in practice. I get that it's beneficial in theory, but<br>
>>>> > that's true of many warnings.<br>
>>>> ><br>
>>>> > Based on how this warning does in practice, I think it might be better<br>
>>>> > for the static analyzer, which has a lower bar for false positives.<br>
>>>><br>
>>>> Noisy in the sense that it correctly diagnoses a self-assignment where<br>
>>>> one **intended** to have self-assignment.<br>
>>>> And unsurprisingly, it happened in the unit-tests, as was expected ^ in<br>
>>>> previous comments.<br>
>>>> **So far** there are no truly false-positives noise (at least no reports<br>
>>>> of it).<br>
>>>><br>
>>>> We could help workaround that the way i initially suggested, by keeping<br>
>>>> this new part of the diag under it's own sub-flag,<br>
>>>> and suggest to disable it for tests. But yes, that<br>
>>>><br>
>>>><br>
>>>><br>
>>>> Repository:<br>
>>>> rC Clang<br>
>>>><br>
>>>> <a href="https://reviews.llvm.org/D44883" rel="noreferrer" target="_blank">https://reviews.llvm.org/D44883</a><br>
>>>><br>
>>>><br>
>>>><br>
>>>> _______________________________________________<br>
>>>> cfe-commits mailing list<br>
>>>> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
>>>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div></blockquote></div>
</blockquote></div>