[llvm] r329491 - [unittests] ADT: silence -Wself-assign diagnostics

[Roman Lebedev via llvm-commits]

Author: lebedevri
Date: Sat Apr  7 03:37:18 2018
New Revision: 329491

URL: http://llvm.org/viewvc/llvm-project?rev=329491&view=rev
Log:
[unittests] ADT: silence -Wself-assign diagnostics

Summary:
D44883 extends -Wself-assign to also work on C++ classes.
In it's current state (as suggested by @rjmccall), it is not under it's own sub-group.
Since that diag is enabled by `-Wall`, stage2 testing showed that:
* It does not fire on any llvm code
* It does fire for these 3 unittests
* It does fire for libc++ tests

This diff simply silences those new warnings in llvm's unittests.
A similar diff will be needed for libcxx. (`libcxx/test/std/language.support/support.types/byteops/`, maybe something else)

Since i don't think we want to repeat rL322901, let's talk about it.
I've subscribed everyone who i think might be interested...

There are several ways forward:
* Not extend -Wself-assign, close D44883. Not very productive outcome i'd say.
* Keep D44883 in it's current state.
  Unless your custom overloaded operators do something unusual for when self-assigning,
  the warning is no less of a false-positive than the current -Wself-assign.
  Except for tests of course, there you'd want to silence it. The current suggestion is:
  ```
  S a;
  a = (S &)a;
  ```
* Split the diagnostic in two - `-Wself-assign-builtin` (i.e. what is `-Wself-assign` in trunk),
  and `-Wself-assign-overloaded` - the new part in D44883.
  Since, as i said, i'm not really sure why it would be less of a error than the current `-Wself-assign`,
  both would still be in `-Wall`. That way one could simply pass `-Wno-self-assign-overloaded` for all the tests.
  Pretty simple to do, and will surely work.
* Split the diagnostic in two - `-Wself-assign-trivial`, and `-Wself-assign-nontrivial`.
  The choice of which diag to emit would depend on trivial-ness of that particular operator.
  The current `-Wself-assign` would be `-Wself-assign-trivial`.
  https://godbolt.org/g/gwDASe - `A`, `B` and `C` case would be treated as trivial, and `D`, `E` and `F` as non-trivial.
  Will be the most complicated to implement.

Thoughts?

Reviewers: aaron.ballman, rsmith, rtrieu, rjmccall, dblaikie, atrick, gottesmm

Reviewed By: dblaikie

Subscribers: lebedev.ri, phosek, vsk, rnk, thakis, sammccall, mclow.lists, llvm-commits, rjmccall

Differential Revision: https://reviews.llvm.org/D45082

Modified:
    llvm/trunk/unittests/ADT/DenseMapTest.cpp
    llvm/trunk/unittests/ADT/SmallPtrSetTest.cpp
    llvm/trunk/unittests/ADT/SparseBitVectorTest.cpp

Modified: llvm/trunk/unittests/ADT/DenseMapTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/DenseMapTest.cpp?rev=329491&r1=329490&r2=329491&view=diff
==============================================================================
--- llvm/trunk/unittests/ADT/DenseMapTest.cpp (original)
+++ llvm/trunk/unittests/ADT/DenseMapTest.cpp Sat Apr  7 03:37:18 2018
@@ -247,7 +247,7 @@ TYPED_TEST(DenseMapTest, AssignmentTest)
   EXPECT_EQ(this->getValue(), copyMap[this->getKey()]);
 
   // test self-assignment.
-  copyMap = copyMap;
+  copyMap = static_cast<TypeParam &>(copyMap);
   EXPECT_EQ(1u, copyMap.size());
   EXPECT_EQ(this->getValue(), copyMap[this->getKey()]);
 }
@@ -262,7 +262,7 @@ TYPED_TEST(DenseMapTest, AssignmentTestN
     EXPECT_EQ(this->getValue(Key), copyMap[this->getKey(Key)]);
 
   // test self-assignment.
-  copyMap = copyMap;
+  copyMap = static_cast<TypeParam &>(copyMap);
   EXPECT_EQ(5u, copyMap.size());
   for (int Key = 0; Key < 5; ++Key)
     EXPECT_EQ(this->getValue(Key), copyMap[this->getKey(Key)]);

Modified: llvm/trunk/unittests/ADT/SmallPtrSetTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/SmallPtrSetTest.cpp?rev=329491&r1=329490&r2=329491&view=diff
==============================================================================
--- llvm/trunk/unittests/ADT/SmallPtrSetTest.cpp (original)
+++ llvm/trunk/unittests/ADT/SmallPtrSetTest.cpp Sat Apr  7 03:37:18 2018
@@ -28,7 +28,7 @@ TEST(SmallPtrSetTest, Assignment) {
   (s2 = s1).insert(&buf[2]);
 
   // Self assign as well.
-  (s2 = s2).insert(&buf[3]);
+  (s2 = static_cast<SmallPtrSet<int *, 4> &>(s2)).insert(&buf[3]);
 
   s1 = s2;
   EXPECT_EQ(4U, s1.size());
@@ -56,7 +56,7 @@ TEST(SmallPtrSetTest, GrowthTest) {
 
   SmallPtrSet<int *, 4> s;
   typedef SmallPtrSet<int *, 4>::iterator iter;
-  
+
   s.insert(&buf[0]);
   s.insert(&buf[1]);
   s.insert(&buf[2]);

Modified: llvm/trunk/unittests/ADT/SparseBitVectorTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/SparseBitVectorTest.cpp?rev=329491&r1=329490&r2=329491&view=diff
==============================================================================
--- llvm/trunk/unittests/ADT/SparseBitVectorTest.cpp (original)
+++ llvm/trunk/unittests/ADT/SparseBitVectorTest.cpp Sat Apr  7 03:37:18 2018
@@ -68,7 +68,7 @@ TEST(SparseBitVectorTest, SelfAssignment
 
   Vec.set(23);
   Vec.set(234);
-  Vec = Vec;
+  Vec = static_cast<SparseBitVector<> &>(Vec);
   EXPECT_TRUE(Vec.test(23));
   EXPECT_TRUE(Vec.test(234));