Page MenuHomePhabricator

[Sema] Add -Wno-self-assign-overloaded
ClosedPublic

Authored by lebedev.ri on Apr 18 2018, 5:52 AM.

Details

Summary

It seems there isn't much enthusiasm for -wtest D45685.

This is more conservative version, which i had in the very first
revision of D44883, but that 'erroneously' got removed because of the review.

Based on some [irc] discussions, it must really be documented that
we want all the new diagnostics to have their own flags, to ease
rollouts, transitions, etc.

Please do note that i'm only adding -Wno-self-assign-overloaded,
but not -Wno-self-assign-field-overloaded, because i'm honestly
not aware of any false-positives from the -field variant,
but i can just as easily add it if wanted.
https://reviews.llvm.org/D44883#1068561

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Apr 18 2018, 5:52 AM

Ping. At least one of these needs to land.

dblaikie added inline comments.Apr 23 2018, 1:02 PM
lib/Sema/SemaExpr.cpp
11527–11528 ↗(On Diff #142917)

Presumably this also changes how the warning is enabled? But that doesn't seem to be tested in this patch?

lebedev.ri added inline comments.Apr 23 2018, 1:10 PM
lib/Sema/SemaExpr.cpp
11527–11528 ↗(On Diff #142917)

What testing do you have in mind?
The test/SemaCXX/warn-self-assign-overloaded.cpp change was supposed to show how it is enabled..

dblaikie accepted this revision.Apr 23 2018, 1:14 PM
dblaikie added inline comments.
lib/Sema/SemaExpr.cpp
11527–11528 ↗(On Diff #142917)

ah, misread those - figured they were testing the negative case (given the name of this patch) but I see they're testing the positive case.

Maybe testing the negative case would be useful too?

This revision is now accepted and ready to land.Apr 23 2018, 1:14 PM

Add negative tests, too.

Closed by commit rL330651: [Sema] Add -Wno-self-assign-overloaded (authored by lebedevri, committed by ). · Explain WhyApr 23 2018, 2:38 PM
This revision was automatically updated to reflect the committed changes.
lebedev.ri added inline comments.Apr 23 2018, 2:39 PM
lib/Sema/SemaExpr.cpp
11527–11528 ↗(On Diff #142917)

Added & committed.
Thank you for the review!

FWIW I don't fundamentalyl object to also having something like -wtest.
Probably needs a better name though (unfortunately the double-negative gets
confusing... - like you want to describe the set of diagnostics that should
not be used in test code, so that as a group might be "-Wnon-test" but then
"-Wno-non-test" is pretty awkward) - probably worth chatting to Richard
Smith about that, I reckon.

FWIW I don't fundamentalyl object to also having something like -wtest.
Probably needs a better name though (unfortunately the double-negative gets
confusing... - like you want to describe the set of diagnostics that should
not be used in test code, so that as a group might be "-Wnon-test" but then
"-Wno-non-test" is pretty awkward) - probably worth chatting to Richard
Smith about that, I reckon.

That's why I was suggesting putting it in the -w namespace. We really wouldn't expect or want users to ever use a *positive* version of this warning option — specifically asking for just the warnings that are known to be problematic for test code, across all warnings. It's just not really a warning group.

It could also be something like -fsuppress-problematic-test-warnings, of course, but I was basically thinking of -w as meaning -fsuppress-problematic-*-warnings.

Is there anything else in the "-w" namespace other than the literal "-w" so
far?

I mean, I could imagine it might make more sense to default these warnings
off & users can turn them on for non-test code, potentially? So
"-Wnon-test" might make sense.

Is there anything else in the "-w" namespace other than the literal "-w" so
far?

No. This would be novel.

I mean, I could imagine it might make more sense to default these warnings
off & users can turn them on for non-test code, potentially? So
"-Wnon-test" might make sense.

That's an interesting idea, but it's still not a warning group, because you shouldn't get the self-assign warnings unless -Wself-assign is enabled.

Hi,

we see the false-positive behavior of -Wno-self-assign-overloaded flag in case of subtraction assignment operator.
The minimal reproducer that we got: https://godbolt.org/g/8PQMpR

typedef int Int_t;
typedef double Double_t;
class TObject {};
template <class Element> class TMatrixTBase : public TObject {};
template <class Element> class TMatrixT : public TMatrixTBase<Element> {
public:
  enum EMatrixCreatorsOp1 {
    kZero,
    kUnit,
    kTransposed,
  };
  TMatrixT(){};
  TMatrixT(Int_t row_lwb, Int_t row_upb, Int_t col_lwb, Int_t col_upb){};
  TMatrixT(EMatrixCreatorsOp1 op, const TMatrixT &prototype){};
  TMatrixT<Element> &operator-=(const TMatrixT<Element> &source){return *this;};
};

typedef TMatrixT<Double_t> TMatrixD;
void stress_binary_ebe_op(Int_t rsize, Int_t csize) {
  TMatrixD m(2, rsize + 1, 0, csize - 1);
  TMatrixD m1(TMatrixD::kZero, m);
  m1 -= m1;
}

Thanks in advance!

Hi,

Hi.

we see the false-positive behavior of -Wno-self-assign-overloaded flag in case of subtraction assignment operator.
The minimal reproducer that we got: https://godbolt.org/g/8PQMpR
...

Could you please clarify, what is the false-positive?
The fact that it fires on overloaded -= is intended behavior, and is tested in tests, see cfe/trunk/test/SemaCXX/warn-self-assign-overloaded.cpp:

#ifndef DUMMY
  a *= a;
  a /= a; // expected-warning {{explicitly assigning}}
  a %= a; // expected-warning {{explicitly assigning}}
  a += a;
  a -= a; // expected-warning {{explicitly assigning}}
  a <<= a;
  a >>= a;
  a &= a; // expected-warning {{explicitly assigning}}
  a |= a; // expected-warning {{explicitly assigning}}
  a ^= a; // expected-warning {{explicitly assigning}}
#endif

Thanks in advance!

Thank you for the test example! I got your point, but I wanted to ask if it should be like this for commutative operations?
In our case it is actually matrix, and subtraction of matrices is not commutative operation..

Thank you for the test example! I got your point, but I wanted to ask if it should be like this for commutative operations?

In our case it is actually matrix, and subtraction of matrices is not commutative operation..

With that context, i do agree that it is a false-positive.
Please file a bug (do we already have a 7.0 metabug yet?).

I'm not too sure what is the best way to solve this.
The most obvious solution would be taking the operator status into account,
like it was considered during review, https://reviews.llvm.org/D44883#1048520,
https://godbolt.org/g/7K7Uwy - and if it is "non_trivial",
then issue it under -Wself-assign-overloaded-nontrivial ?

Thank you for the test example! I got your point, but I wanted to ask if it should be like this for commutative operations?
In our case it is actually matrix, and subtraction of matrices is not commutative operation..

Mathematical commutativity has nothing to do with this diagnostic. The diagnostic is complaining that m1 -= m1 produces a trivial effect (in this case m1.clear()) via a syntax that might indicate that the programmer made a typo (i.e. the compiler suspects that you meant m -= m1 or something).
From the function name stress_..., I infer that this is a unit-test. This warning is known to give "false positives" in unit-test code, where one often actually wants to achieve trivial effects through complicated code. The current recommendation there, AFAIK, is either to write something like

m1 -= static_cast<TMatrixD&>(m1);  // hide the self-subtraction from the compiler

or else to add -Wno-self-assign-overloaded to your compiler flags. (libc++'s test suite already passes -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-conversion -Wno-unused-local-typedef -Wno-#warnings, for example.)
(That said, I continue to think that this diagnostic produces more noise than signal, and wish it weren't in -Wall...)

Thank you for the test example! I got your point, but I wanted to ask if it should be like this for commutative operations?
In our case it is actually matrix, and subtraction of matrices is not commutative operation..

Mathematical commutativity has nothing to do with this diagnostic. The diagnostic is complaining that m1 -= m1 produces a trivial effect (in this case m1.clear()) via a syntax that might indicate that the programmer made a typo (i.e. the compiler suspects that you meant m -= m1 or something).
From the function name stress_..., I infer that this is a unit-test. This warning is known to give "false positives" in unit-test code, where one often actually wants to achieve trivial effects through complicated code. The current recommendation there, AFAIK, is either to write something like

m1 -= static_cast<TMatrixD&>(m1);  // hide the self-subtraction from the compiler

or else to add -Wno-self-assign-overloaded to your compiler flags. (libc++'s test suite already passes -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-conversion -Wno-unused-local-typedef -Wno-#warnings, for example.)
(That said, I continue to think that this diagnostic produces more noise than signal, and wish it weren't in -Wall...)

Thanks for your opinion, your guess was right! It is a unit-test, so seems we need to try to suppress warnings on our side.

Thanks for your opinion, your guess was right! It is a unit-test, so seems we need to try to suppress warnings on our side.

Ok, great that this time it got resolved this trivially!

(That said, I continue to think that this diagnostic produces more noise than signal, and wish it weren't in -Wall...)

Too bad there is no way to track how many true-positives it produces.
Anecdotal evidence: it just prevented me from wasting time trying to understand what is going on during D46602 development, so sure, it can have false-positive noise, but..