<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Tue, Apr 10, 2018 at 7:21 AM Roman Lebedev via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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 warning :-P It fired 8 times, all false positives, and all from unit tests testing that operator= works for self-assignment. (<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 exact details) It looks like the same issue exists in LLVM itself too, <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, good to know...<br>
<br>
> 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.<br>
><br>
> 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.<br>
<br>
Noisy in the sense that it correctly diagnoses a self-assignment where one **intended** to have self-assignment.<br>
And unsurprisingly, it happened in the unit-tests, as was expected ^ in previous comments.<br>
**So far** there are no truly false-positives noise (at least no reports of it).<br></blockquote><div><br>False positive in the broad sense (which I think I mentioned in review of this patch) of "diagnosing code that is working as intended (rather than diagnosing/identifying bugs/mistakes) & is fairly unsurprising/legible (ie: the warning didn't find places that weren't bugs, but were sufficiently confusing that they could be written better some other way)".<br><br>In the past, the bar for warnings has been higher in my experience (though an interesting argument about whether making a change to an existing warning, versus adding a warning under a separate flag, changes the decision (since it may wash out new false negatives with the many true negatives in the original implementation)).<br><br>Probably one of the best ways to evaluate whether this is beneficial in practice is to leave it in (at least inside Google or anywhere else that can track diagnostic frequency) for a while & see how often it comes up in practice... - though I don't know if we'd be able to separate out the cases that come from this improvement versus all the rest - maybe if the text is (or was made to be) worded slightly differently, or if it were under a separate flag.<br><br>(but yes, I recall the discussion about flag naming and subsetting the flags, etc - I don't have a good answer to that)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">We could help workaround that the way i initially suggested, by keeping 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>
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>
</blockquote></div></div>