In the interest of completeness, we now had our first true positive with this warning, here: https://github.com/KhronosGroup/VK-GL-CTS/blob/master/framework/referencerenderer/rrFragmentOperations.cpp#L603 (and another self-assign operator= unittest in that repo).
On Fri, Apr 13, 2018 at 3:00 AM, John McCall <rjmcc...@gmail.com> wrote: > (Sorry for the delay in responding — I'm actually on vacation.) > > On Tue, Apr 10, 2018 at 1:52 PM, David Blaikie <dblai...@gmail.com> wrote: > >> 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. >> > > True. I agree that -Wparentheses is really at least three and probably > four separate warnings, all bundled into one flag. I was really only > thinking about the = for == warning, which is an important warning that > ends up being highly opinionated about how you write your code. > > 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". >> > > I think you might be misunderstanding -Wparentheses as primarily a warning > about confusing precedence rather than primarily a warning about > accidentally substituting = for ==. This of course interacts with > precedence for the conditional operator because the assignment is no longer > parsed within the condition. Admittedly, GCC's documentation doesn't > explain this well. > > >> 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? >> > > I see. 8 did seem rather low for all of Google. And all 100-200 are > false positives? Or have only the Chromium cases been investigated yet? > > 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) >> > > I understand. > > I still believe strongly that we should be warning here, but I'm certainly > open to allowing it to be disabled. We did actually talk about this during > the review. There are three general options here, not necessary exclusive: > 1. We move it to a separate warning group. I would argue that (a) this > should at least be a sub-group so that -Wself-assign turns it on by > default, and that (b) trivial self-assignments should still be triggered > under -Wself-assign, for C/C++ consistency if nothing else. > 2. We find some way to turn it off by recognizing something about the code > that suggests that we're in test code. > 3. We add a general -wtest (capitalization intentional) that says "this is > test code, please don't warn about code patterns that would be unusual in > normal code but might make sense in tests". I'm positive I've seen other > examples of that, e.g. for some of our warnings around misuse of library > functions. Many build systems make it fairly early to use different build > settings when building tests — IIRC that's true of Bazel. > > John. >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits