[PATCH] D45082: [RFC][unittests] ADT: silence -Wself-assign diagnostics

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: aaron.ballman, rsmith, rtrieu, rjmccall, dblaikie.

https://reviews.llvm.org/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 https://reviews.llvm.org/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 https://reviews.llvm.org/D44883. Not very productive outcome i'd say.
- Keep https://reviews.llvm.org/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 https://reviews.llvm.org/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?


Repository:
  rL LLVM

https://reviews.llvm.org/D45082

Files:
  unittests/ADT/DenseMapTest.cpp
  unittests/ADT/SmallPtrSetTest.cpp
  unittests/ADT/SparseBitVectorTest.cpp


Index: unittests/ADT/SparseBitVectorTest.cpp
===================================================================
--- unittests/ADT/SparseBitVectorTest.cpp
+++ unittests/ADT/SparseBitVectorTest.cpp
@@ -68,7 +68,7 @@
 
   Vec.set(23);
   Vec.set(234);
-  Vec = Vec;
+  Vec = (SparseBitVector<> &)Vec; // the cast is to avoid the warning.
   EXPECT_TRUE(Vec.test(23));
   EXPECT_TRUE(Vec.test(234));
 
Index: unittests/ADT/SmallPtrSetTest.cpp
===================================================================
--- unittests/ADT/SmallPtrSetTest.cpp
+++ unittests/ADT/SmallPtrSetTest.cpp
@@ -27,8 +27,8 @@
   SmallPtrSet<int *, 4> s2;
   (s2 = s1).insert(&buf[2]);
 
-  // Self assign as well.
-  (s2 = s2).insert(&buf[3]);
+  // Self assign as well. The cast is to avoid the warning.
+  (s2 = (SmallPtrSet<int *, 4> &)s2).insert(&buf[3]);
 
   s1 = s2;
   EXPECT_EQ(4U, s1.size());
@@ -56,7 +56,7 @@
 
   SmallPtrSet<int *, 4> s;
   typedef SmallPtrSet<int *, 4>::iterator iter;
-  
+
   s.insert(&buf[0]);
   s.insert(&buf[1]);
   s.insert(&buf[2]);
Index: unittests/ADT/DenseMapTest.cpp
===================================================================
--- unittests/ADT/DenseMapTest.cpp
+++ unittests/ADT/DenseMapTest.cpp
@@ -246,8 +246,8 @@
   EXPECT_EQ(1u, copyMap.size());
   EXPECT_EQ(this->getValue(), copyMap[this->getKey()]);
 
-  // test self-assignment.
-  copyMap = copyMap;
+  // test self-assignment. the cast is to avoid the warning.
+  copyMap = (TypeParam &)copyMap;
   EXPECT_EQ(1u, copyMap.size());
   EXPECT_EQ(this->getValue(), copyMap[this->getKey()]);
 }
@@ -261,8 +261,8 @@
   for (int Key = 0; Key < 5; ++Key)
     EXPECT_EQ(this->getValue(Key), copyMap[this->getKey(Key)]);
 
-  // test self-assignment.
-  copyMap = copyMap;
+  // test self-assignment. the cast is to avoid the warning.
+  copyMap = (TypeParam &)copyMap;
   EXPECT_EQ(5u, copyMap.size());
   for (int Key = 0; Key < 5; ++Key)
     EXPECT_EQ(this->getValue(Key), copyMap[this->getKey(Key)]);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D45082.140407.patch
Type: text/x-patch
Size: 1985 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180330/f48c2613/attachment.bin>