<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Tue, Apr 10, 2018 at 10:20 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">Do you think they’re bad precedent?</blockquote><div><br>Somewhat, yes - though -Wparens is perhaps conflating a few cases too. I think the argument for the -Wparens for precedence is probably pretty good.<br><br>But the argument for -Wparens for conditional assignments would probably be pretty hard to make today, in a codebase that'd never seen the warning before - the false positive rate would probably be really high & not something that would be reasonable to expect people to do all the cleanup for. (that's been the general bar "is the value gained worth the cleanup" - this precludes a lot of warnings that would probably be acceptable in new code (where the incremental cost of keeping in compliance with the warning wouldn't be so bad - but the cleanup cost in an existing codebase would be quite high/churny (admittedly, as we've gotten better tooling for wide scale changes/refactoring, this may've changed the balance a bit - but those tools aren't exactly super widely available/easy to plug in)))<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Do you have a replacement for that approach to warning about those problems? </blockquote><div><br>If we were looking at a green field today (code that'd never seen the warning before), I don't think -Wparens for assignment in conditional would pass the bar (or at least the bar as Clang seemed to have 6-7 years ago when I would talk to Doug about warning ideas, as best as I understood the perspective being espoused back then). I suspect maybe in that world we would've found other warnings with lower-false-positive that might've been able to diagnose assignment-in-conditional a bit more precisely than "all assignments are so suspect you have to manually go and look at them to decide".<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Because they certainly were precedent for -Wfallthrough, and they certainly catch a class of bugs at least as large and important as that warning, and they certainly have exactly the same false-positive characteristics as it does. I am really struggling to see a difference in kind or even degree here.<br><br>-Wself-assign is a much less important warning but it’s also far less likely to see false positives, except in this pattern of unit tests for data structures, which are not commonly-written code. As is, in fact, evidenced by Google, a company full of programmers whom I think I can fairly but affectionately describe as loving to reinvent data structures, only seeing this warning eight times.<br></blockquote><div><br>I think the 8 cases were in Chromium - at Google so far (& I'm not sure if that's across all of Google's codebase, or some subset) something in the 100-200 cases?<br><br>Nico's point that, so far, the only evidence we have is that this warning added false positives and didn't catch any real world bugs. Yeah, a small number/easy enough to cleanup, but all we have is the cost and no idea of the value. (usually when we roll out warnings - even the -Wparens and -Wfallthrough, we find checked in code that has those problems (& the false positive cases) & so we evaluate cost/benefit with that data)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>John. <br><div class="gmail_quote"><div dir="ltr">On Tue, Apr 10, 2018 at 19:03 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:59 AM John McCall <<a href="mailto:rjmccall@gmail.com" target="_blank">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">Also, the standard for the static analyzer is not lower than it is for the compiler.<br><br>The static analyzer tries to catch a much larger class of bugs, but it also tries very hard to make all the warnings it emits count. There’s a really common problem in the static analysis field where users try a static analyzer, get buried by 50 pages of diagnostics, and immediately throw up their hands and abandon it forever. Our analyzer is very circumspect about false positives specifically to try to fight that.<br><br>In contrast, the compiler is *much* more willing to just tell users that they might have a good reason for doing something but they should write it another way to shut the compiler up. That is the basic design of -Wparens, -Wfallthrough, -Wswitch, and a bunch of other major warnings. I think this is of a piece.<br></blockquote></div></div><div dir="ltr"><div class="gmail_quote"><div><br>In the past -Wparens and -Wswitch haven't been cited as precedent that was desirable to continue. -Wfallthrough was argued for in favor of the large class of bugs it catches.<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">We put warnings in the static analyzer if (1) they’re API-specific in a way that we’re not comfortable with in the compiler or (2) there would be major false positives that we can only suppress with a much more expensive analysis than we’re comfortable with in the compiler.<br><br>I do not see a way that we could suppress this warning in the static analyzer. The static analyzer is unlikely to be able to prove that the assignment is actually idempotent, and it is not going to be able to infer that the code is actually in a unit test for the assigned type any better than the compiler can. <br></blockquote></div></div><div dir="ltr"><div class="gmail_quote"><div><br>Sure, clang-tidy might be a better place for such things.<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"><br>John. <br><div class="gmail_quote"><div dir="ltr">On Tue, Apr 10, 2018 at 18:34 John McCall <<a href="mailto:rjmccall@gmail.com" target="_blank">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>
</blockquote></div></div></blockquote></div>
</blockquote></div></div>