Do you think they’re bad precedent? Do you have a replacement for that approach to warning about those problems? 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. 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