<div dir="ltr">In the interest of completeness, we now had our first true positive with this warning, here: <a href="https://github.com/KhronosGroup/VK-GL-CTS/blob/master/framework/referencerenderer/rrFragmentOperations.cpp#L603">https://github.com/KhronosGroup/VK-GL-CTS/blob/master/framework/referencerenderer/rrFragmentOperations.cpp#L603</a> (and another self-assign operator= unittest in that repo).</div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Apr 13, 2018 at 3:00 AM, John McCall <span dir="ltr"><<a href="mailto:rjmccall@gmail.com" target="_blank">rjmccall@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>(Sorry for the delay in responding — I'm actually on vacation.)</div><span class=""><div><br></div>On Tue, Apr 10, 2018 at 1:52 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br></span><div class="gmail_extra"><div class="gmail_quote"><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">On Tue, Apr 10, 2018 at 10:20 AM John McCall <<a href="mailto:rjmccall@gmail.com" target="_blank">rjmccall@gmail.com</a>> wrote:<br><div class="gmail_quote"><span><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></span><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></div></div></div></blockquote><div><br></div></span><div>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.</div><span class=""><div><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"><span><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></span><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></div></div></blockquote><div><br></div></span><div>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.</div><span class=""><div> </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"><span><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></span><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></div></div></div></blockquote><div><br></div></span><div>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?</div><span class=""><div><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>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></div></div></blockquote><div><br></div></span><div>I understand.</div><div><br></div><div>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:</div><div>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.</div><div>2. We find some way to turn it off by recognizing something about the code that suggests that we're in test code.</div><div>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.</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>John.</div></font></span></div></div></div>
</blockquote></div><br></div>