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