From 429b00325ffc64168628a8bab38b1df3c4d5a3ae Mon Sep 17 00:00:00 2001 From: Roman Lebedev Date: Sat, 7 Apr 2018 10:37:18 +0000 Subject: [PATCH] [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 llvm-svn: 329491 --- llvm/unittests/ADT/DenseMapTest.cpp | 4 ++-- llvm/unittests/ADT/SmallPtrSetTest.cpp | 4 ++-- llvm/unittests/ADT/SparseBitVectorTest.cpp | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/llvm/unittests/ADT/DenseMapTest.cpp b/llvm/unittests/ADT/DenseMapTest.cpp index 299d5019..7f52c54 100644 --- a/llvm/unittests/ADT/DenseMapTest.cpp +++ b/llvm/unittests/ADT/DenseMapTest.cpp @@ -247,7 +247,7 @@ TYPED_TEST(DenseMapTest, AssignmentTest) { EXPECT_EQ(this->getValue(), copyMap[this->getKey()]); // test self-assignment. - copyMap = copyMap; + copyMap = static_cast(copyMap); EXPECT_EQ(1u, copyMap.size()); EXPECT_EQ(this->getValue(), copyMap[this->getKey()]); } @@ -262,7 +262,7 @@ TYPED_TEST(DenseMapTest, AssignmentTestNotSmall) { EXPECT_EQ(this->getValue(Key), copyMap[this->getKey(Key)]); // test self-assignment. - copyMap = copyMap; + copyMap = static_cast(copyMap); EXPECT_EQ(5u, copyMap.size()); for (int Key = 0; Key < 5; ++Key) EXPECT_EQ(this->getValue(Key), copyMap[this->getKey(Key)]); diff --git a/llvm/unittests/ADT/SmallPtrSetTest.cpp b/llvm/unittests/ADT/SmallPtrSetTest.cpp index a428767..76f9cf7 100644 --- a/llvm/unittests/ADT/SmallPtrSetTest.cpp +++ b/llvm/unittests/ADT/SmallPtrSetTest.cpp @@ -28,7 +28,7 @@ TEST(SmallPtrSetTest, Assignment) { (s2 = s1).insert(&buf[2]); // Self assign as well. - (s2 = s2).insert(&buf[3]); + (s2 = static_cast &>(s2)).insert(&buf[3]); s1 = s2; EXPECT_EQ(4U, s1.size()); @@ -56,7 +56,7 @@ TEST(SmallPtrSetTest, GrowthTest) { SmallPtrSet s; typedef SmallPtrSet::iterator iter; - + s.insert(&buf[0]); s.insert(&buf[1]); s.insert(&buf[2]); diff --git a/llvm/unittests/ADT/SparseBitVectorTest.cpp b/llvm/unittests/ADT/SparseBitVectorTest.cpp index 6cd4de3..9d6f4f1 100644 --- a/llvm/unittests/ADT/SparseBitVectorTest.cpp +++ b/llvm/unittests/ADT/SparseBitVectorTest.cpp @@ -68,7 +68,7 @@ TEST(SparseBitVectorTest, SelfAssignment) { Vec.set(23); Vec.set(234); - Vec = Vec; + Vec = static_cast &>(Vec); EXPECT_TRUE(Vec.test(23)); EXPECT_TRUE(Vec.test(234)); -- 2.7.4