On Tue, Apr 10, 2018 at 10:20 AM John McCall <rjmcc...@gmail.com> wrote:
> Do you think they’re bad precedent? Somewhat, yes - though -Wparens is perhaps conflating a few cases too. I think the argument for the -Wparens for precedence is probably pretty good. 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))) > Do you have a replacement for that approach to warning about those > problems? 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". > 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. > > -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. > 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? 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) > > John. > On Tue, Apr 10, 2018 at 19:03 David Blaikie <dblai...@gmail.com> wrote: > >> On Tue, Apr 10, 2018 at 9:59 AM John McCall <rjmcc...@gmail.com> wrote: >> >>> Also, the standard for the static analyzer is not lower than it is for >>> the compiler. >>> >>> 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. >>> >>> 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. >>> >> >> 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. >> >> >>> 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. >>> >>> 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. >>> >> >> Sure, clang-tidy might be a better place for such things. >> >> >>> >>> John. >>> On Tue, Apr 10, 2018 at 18:34 John McCall <rjmcc...@gmail.com> wrote: >>> >>>> 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. >>>> >>>> John. >>>> On Tue, Apr 10, 2018 at 18:27 David Blaikie <dblai...@gmail.com> wrote: >>>> >>>>> On Tue, Apr 10, 2018 at 9:25 AM Roman Lebedev <lebedev...@gmail.com> >>>>> wrote: >>>>> >>>>>> Because *so far* it has been running on the existing code, which i >>>>>> guess >>>>>> was already pretty warning free, and was run through the static >>>>>> analyzer, >>>>> >>>>> which obviously should catch such issues. >>>>>> >>>>> >>>>> 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)). >>>>> >>>>> >>>>>> Do you always use [clang] static analyzer, each time you build? >>>>>> Great! It is indeed very good to do so. But does everyone? >>>>>> >>>>>> This particular issue is easily detectable without full static >>>>>> analysis, >>>>>> and as it is seen from the differential, was very straight-forward to >>>>>> implement >>>>>> as a simple extension of pre-existing -Wself-assign. >>>>>> >>>>>> So if this is really truly unacceptable false-positive rate, ok, sure, >>>>>> file a RC bug, >>>>>> and i will split it so that it can be simply disabled for *tests*. >>>>>> >>>>>> That being said, i understand the reasons behind "keep clang >>>>>> diagnostics >>>>>> false-positive free". I don't really want to change that. >>>>>> >>>>>> On Tue, Apr 10, 2018 at 7:13 PM, David Blaikie <dblai...@gmail.com> >>>>>> wrote: >>>>>> > 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 <rjmcc...@gmail.com> >>>>>> 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 <tha...@chromium.org> >>>>>> 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 <cfe-commits@lists.llvm.org> wrote: >>>>>> >>>> >>>>>> >>>> lebedev.ri added a comment. >>>>>> >>>> >>>>>> >>>> In https://reviews.llvm.org/D44883#1063003, @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. >>>>>> >>>> > ( >>>>>> https://chromium-review.googlesource.com/c/chromium/src/+/1000856 >>>>>> has the >>>>>> >>>> > exact details) It looks like the same issue exists in LLVM >>>>>> itself too, >>>>>> >>>> > https://reviews.llvm.org/D45082 >>>>>> >>>> >>>>>> >>>> >>>>>> >>>> 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 >>>>>> >>>> >>>>>> >>>> https://reviews.llvm.org/D44883 >>>>>> >>>> >>>>>> >>>> >>>>>> >>>> >>>>>> >>>> _______________________________________________ >>>>>> >>>> cfe-commits mailing list >>>>>> >>>> cfe-commits@lists.llvm.org >>>>>> >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>>>>> >>>>>
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
