[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

[Nico Weber via cfe-commits]

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 <rjmccall at 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 <dblaikie at gmail.com> wrote:
>
>> On Tue, Apr 10, 2018 at 10:20 AM John McCall <rjmccall at 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180413/3079072d/attachment-0001.html>