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

On Mon, Apr 16, 2018 at 5:11 AM, Roman Lebedev via Phabricator via cfe-commits wrote: > lebedev.ri added a comment. > > Re false-positives - at least two [post-]reviewers need to agree on the > way forward (see previous comments, mail thread), and then i will

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

lebedev.ri added a comment. In https://reviews.llvm.org/D44883#1068602, @lebedev.ri wrote: > There are several options: > > 1. @rjmccall's idea: `-wtest` (lowercase), which in this case will disable > that new code in `BuildOverloadedBinOp()`. i quite like it actually. > 2. split it up like i

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

lebedev.ri added a comment. There are several options: 1. @rjmccall's idea: `-wtest` (lowercase), which in this case will disable that new code in `BuildOverloadedBinOp()`. i quite like it actually. 2. split it up like i had in the first revision - ``-Wself-assign-builtin``,

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

chandlerc added a comment. These kinds of improvements to warnings are awesome, but the way we are deploying them presents serious challenges to adoption which I think we need to address. I think significant new warning functionality should as a matter of course go behind some warning group

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

lebedev.ri added a comment. Re false-positives - at least two [post-]reviewers need to agree on the way forward (see previous comments, mail thread), and then i will implement it. In https://reviews.llvm.org/D44883#1068576, @brooksmoses wrote: > A further concern about this in the general case

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

brooksmoses added a comment. A further concern about this in the general case from the reviewer of one of my test-cleanup changes: The "var = *" idiom is not necessarily equivalent to "var = var" in cases of user-defined types, because operator& may be overloaded. Repository: rC Clang

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

brooksmoses added a comment. Some further statistics, now that I've done a full cleanup on our code: 8 actual bugs newly found by -Wself-assign-field. (Thank you!) 2 actual bugs newly found by -Wself-assign 6 instances of redundant code newly found by -Wself-assign 90 (approximately) instances

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

lebedev.ri added a comment. In https://reviews.llvm.org/D44883#1068400, @brooksmoses wrote: > I have noticed two things when attempting to release LLVM with this revision > internally at Google: > > 1. It's catching real bugs, all in constructors where someone wrote "member_ > = member_" when

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

brooksmoses added a comment. Further note: I'm noticing that nearly all the signal is from -Wself-assign-field and all the noise is from -Wself-assign. So that would be one obvious way to make this higher-signal in what's enabled in -Wall, if that were a desire. Repository: rC Clang

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

brooksmoses added a comment. I have noticed two things when attempting to release LLVM with this revision internally at Google: 1. It's catching real bugs, all in constructors where someone wrote "member_ = member_" when they meant "member_ = member". 2. It's catching at least as many cases

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

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,

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

(Sorry for the delay in responding — I'm actually on vacation.) On Tue, Apr 10, 2018 at 1:52 PM, David Blaikie wrote: > On Tue, Apr 10, 2018 at 10:20 AM John McCall wrote: > >> Do you think they’re bad precedent? > > > Somewhat, yes - though -Wparens is

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

On Tue, Apr 10, 2018 at 10:20 AM John McCall 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. But the argument for -Wparens for

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

On Tue, Apr 10, 2018 at 12:59 PM, John McCall wrote: > Also, the standard for the static analyzer is not lower than it is for the > compiler. > > The static analyzer tries to catch a much larger class of bugs, but it > also tries very hard to make all the warnings it emits

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

Do you think they’re bad precedent? Do you have a replacement for that approach to warning about those problems? 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

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

On Tue, Apr 10, 2018 at 9:59 AM John McCall wrote: > Also, the standard for the static analyzer is not lower than it is for the > compiler. > > The static analyzer tries to catch a much larger class of bugs, but it > also tries very hard to make all the warnings it emits

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

Also, the standard for the static analyzer is not lower than it is for the compiler. The static analyzer tries to catch a much larger class of bugs, but it also tries very hard to make all the warnings it emits count. There’s a really common problem in the static analysis field where users try a

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

Any good ideas on how to evaluate it, then? (in addition to/other than something like Google where we can track the diagnostic for - a few months?) On Tue, Apr 10, 2018 at 9:34 AM John McCall wrote: > Yeah, -Wself-assign in general is about catching a class of live-coding >

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

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

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

On Tue, Apr 10, 2018 at 12:13 PM, David Blaikie 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. > Right. We used to hold ourselves to very

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

On Tue, Apr 10, 2018 at 9:25 AM Roman Lebedev 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

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

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. Do you always use [clang] static analyzer, each time you build? Great! It is indeed very good to do so. But

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

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

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

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

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

"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

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

On Tue, Apr 10, 2018 at 7:21 AM Roman Lebedev via Phabricator < revi...@reviews.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

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

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. >

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

thakis added a comment. 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

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

This revision was automatically updated to reflect the committed changes. Closed by commit rC329493: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self… (authored by lebedevri, committed by ). Repository: rC Clang https://reviews.llvm.org/D44883 Files:

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL329493: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self… (authored by lebedevri, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit:

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

lebedev.ri added a comment. In https://reviews.llvm.org/D44883#1054326, @thakis wrote: > In https://reviews.llvm.org/D44883#1048751, @dblaikie wrote: > > > Historically Clang's policy on warnings was, I think, much more > > conservative than it seems to be today. There was a strong desire not

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

lebedev.ri updated this revision to Diff 140787. lebedev.ri added a comment. - Rebased - Silence both of the diagnostics in an unevaluated context. - Fixed `-Wself-assign-field` preexisting problems: - False-positives on `volatile` stores. - Do not warn in template instantiations. - Handle

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

EricWF added a comment. In https://reviews.llvm.org/D44883#1055018, @Quuxplusone wrote: > @EricWF, is it important IYO that this warning not trigger in unevaluated > contexts even for non-dependently-typed variables? > This is the case that seems to be coming up in practice in libc++ tests,

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

rjmccall added a comment. No, the analysis is intentionally syntactic; it should apply even on dependently-typed arguments (but not get re-checked on instantiation). But I agree that the warning ought to be disabled in unevaluated contexts. Repository: rC Clang

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

Quuxplusone added a comment. > The warning is fundamentally about dataflow, but this doesn't apply to > unevaluated expressions. There are plenty of cases where a user might want to > ask if assignment is well formed on noexcept using only one variable. For > example: > > template void

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

EricWF added a comment. One thing I would like to see in this patch is ensuring the warning doesn't get generated when the expression appears in unevaluated contexts, such as `decltype` and `noexcept`. The warning is fundamentally about dataflow, but this doesn't apply to unevaluated

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

rjmccall added a comment. Right. I think it's fair to acknowledge that many data structure unit tests will contain a legitimate use of a user-defined self-assignment without feeling that that disqualifies the warning. Note that the purpose of this kind of breadth testing is just to look for

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

On Mon, Apr 2, 2018 at 8:05 AM Roman Lebedev via Phabricator < revi...@reviews.llvm.org> wrote: > lebedev.ri added a comment. > > In https://reviews.llvm.org/D44883#1054326, @thakis wrote: > > > In https://reviews.llvm.org/D44883#1048751, @dblaikie wrote: > > > > > Historically Clang's policy on

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

On Mon, Apr 2, 2018 at 11:05 AM, Roman Lebedev via Phabricator via cfe-commits wrote: > lebedev.ri added a comment. > > In https://reviews.llvm.org/D44883#1054326, @thakis wrote: > > > In https://reviews.llvm.org/D44883#1048751, @dblaikie wrote: > > > > > Historically

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

lebedev.ri added a comment. In https://reviews.llvm.org/D44883#1054326, @thakis wrote: > In https://reviews.llvm.org/D44883#1048751, @dblaikie wrote: > > > Historically Clang's policy on warnings was, I think, much more > > conservative than it seems to be today. There was a strong desire not

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

On Mon, Apr 2, 2018 at 10:54 AM, Nico Weber via Phabricator via cfe-commits wrote: > thakis added a comment. > > In https://reviews.llvm.org/D44883#1048751, @dblaikie wrote: > > > Historically Clang's policy on warnings was, I think, much more > > conservative than

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

thakis added a comment. In https://reviews.llvm.org/D44883#1048751, @dblaikie wrote: > Historically Clang's policy on warnings was, I think, much more > conservative than it seems to be today. There was a strong desire not to > implement off-by-default warnings, and to have warnings with an >

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

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks. LGTM whenever you've cleared up the self-hosting problems. Repository: rC Clang https://reviews.llvm.org/D44883 ___ cfe-commits

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

lebedev.ri updated this revision to Diff 140554. lebedev.ri added a comment. - Revised release notes entry - Created https://reviews.llvm.org/D45128 with libcxx patch from stage-2 testing. Repository: rC Clang https://reviews.llvm.org/D44883 Files: docs/ReleaseNotes.rst

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

rjmccall added inline comments. Comment at: docs/ReleaseNotes.rst:63 +- ``-Wself-assign`` and ``-Wself-assign-field`` were extended to diagnose + self-assignment operations using overloaded operators (i.e. classes) + Missing a final period. Also, these release

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

lebedev.ri updated this revision to Diff 140547. lebedev.ri added a comment. - Rebased - Also test that `*&` contraption silences the warning. Repository: rC Clang https://reviews.llvm.org/D44883 Files: docs/ReleaseNotes.rst lib/Sema/SemaExpr.cpp

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

lebedev.ri updated this revision to Diff 140411. lebedev.ri added a comment. - Rebased - Created https://reviews.llvm.org/D45082 with llvm diff to prevent stage2 failure, and to disscuss the options. Similar diff will be needed at least for libc++ tests. Repository: rC Clang

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

rjmccall added a comment. In https://reviews.llvm.org/D44883#1051878, @lebedev.ri wrote: > Ok, tried llvm stage2 build, and it failed as expected, in code that > intentionally does self-assignment: Actually, this is not the sort of failure that I think should worry you. Any new warning has

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

lebedev.ri added a comment. Ok, tried llvm stage2 build, and it failed as expected, in code that intentionally does self-assignment: https://github.com/llvm-mirror/llvm/blob/1aa8147054e7ba8f2b7ea25daaed804662b4c6b2/unittests/ADT/DenseMapTest.cpp#L249-L250 [2/311 1.1/sec] Building CXX object

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

rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:12093 +break; + } + lebedev.ri wrote: > rjmccall wrote: > > lebedev.ri wrote: > > > rjmccall wrote: > > > > lebedev.ri wrote: > > > > > rjmccall wrote: > > > > > > I think doing this here can

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

lebedev.ri added a comment. Will do stage2 testing next.. Comment at: lib/Sema/SemaExpr.cpp:12087 + case BO_AndAssign: + case BO_OrAssign: +DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, /*IsBuiltin=*/false); lebedev.ri wrote: > rjmccall wrote: > >

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

rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:12093 +break; + } + lebedev.ri wrote: > rjmccall wrote: > > lebedev.ri wrote: > > > rjmccall wrote: > > > > I think doing this here can result in double-warning if the overload > > > >

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

lebedev.ri added inline comments. Comment at: lib/Sema/SemaExpr.cpp:12093 +break; + } + rjmccall wrote: > lebedev.ri wrote: > > rjmccall wrote: > > > I think doing this here can result in double-warning if the overload > > > resolves to a builtin operator.

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

rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:12093 +break; + } + lebedev.ri wrote: > rjmccall wrote: > > I think doing this here can result in double-warning if the overload > > resolves to a builtin operator. Now, it might not

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

lebedev.ri added inline comments. Comment at: lib/Sema/SemaExpr.cpp:12093 +break; + } + rjmccall wrote: > I think doing this here can result in double-warning if the overload resolves > to a builtin operator. Now, it might not actually be possible for

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

rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:12093 +break; + } + I think doing this here can result in double-warning if the overload resolves to a builtin operator. Now, it might not actually be possible for that to combine with the

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

lebedev.ri updated this revision to Diff 139948. lebedev.ri retitled this revision from "[Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)" to "[Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment