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

John McCall via llvm-commits llvm-commits at lists.llvm.org
Fri Apr 13 00:00:13 PDT 2018


(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/llvm-commits/attachments/20180413/e4ad92ef/attachment.html>


More information about the llvm-commits mailing list