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

[lebedev.ri]

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?

[dblaikie]

Comment might need to be reworded, but might not even need a comment - if
we build with the warning on by default anyone who attempts to remove it
will find out...

& maybe "x = *&x" might be a shorter way to subvert the warning? Not sure
if it's any better.

Alternatively, perhaps a function:

template<typename T>
const T&launder(const T& x) { return x; }

& write a comment on that function explaining why that function exists - to
thwart the -Wself-assign warning.

& then test with: x = launder(x);

or the like...

[lebedev.ri]

• Rebase
• Drop comments
• Silence the warning via "*&" contraption, not by casting to the reference.

The main question still stands, do we want to recommend everyone to use that in their codebases,
or double the amount of diag flags and provide a way to silence this specific diagnostic?

[rjmccall]

In D45082#1053724, @lebedev.ri wrote:

• Rebase
• Drop comments
• Silence the warning via "*&" contraption, not by casting to the reference.

The main question still stands, do we want to recommend everyone to use that in their codebases,
or double the amount of diag flags and provide a way to silence this specific diagnostic?

I think it's probably best to just provide a blessed way of silencing the warning in code and recommend that people adopt that. A good analogue here is the use of unnecessary parentheses to silence the -Wparens warning about using '=' instead of '==' in an if-condition; we've always recognized that there are legitimate reasons to write code that way, so we just picked a code idiom that we promise will always shut the warning off and tell people to use it if they need it.

I think I still prefer casting the RHS as the idiom to bless here, on the grounds that casts are the more universal "treat this as an opaque value" tool; for example, that's how you tell -Wconversion to shut up. On the other hand, *& is certainly not something that we expect to see much of in ordinary code, so it works, and it's definitely a less annoying thing to type.

[lebedev.ri]

If this looks ok, and there are no further unresolved problems here, could someone accept the differential, please? :)

[dblaikie]

That's fair (re: cast over &*)

[lebedev.ri]

And back to using cast instead of *&.

[dblaikie]

Switch this to a C++ style static_cast and go ahead. Thanks!

[lebedev.ri]

Don't use C-style cast, use static_cast<>(), as suggested by @dblaikie

[thakis]

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 details)

Now tests often need warning suppressions for things like this, and this in itself doesn't seem horrible. However, this change takes a warning that was previously 100% noise-free in practice and makes it pretty noisy – without a big benefit in practice. I get that it's beneficial in theory, but that's true of many warnings.

Based on how this warning does in practice, I think it might be better for the static analyzer, which has a lower bar for false positives.

[thakis]

Sorry, that comment was meant for the commit that tweaked the warning in clang. I'll repost there.

llvm/trunk/unittests/ADT/DenseMapTest.cpp

 TYPED_TEST(DenseMapTest, AssignmentTest) {
   this->Map[this->getKey()] = this->getValue();
   TypeParam copyMap = this->Map;
 
   EXPECT_EQ(1u, copyMap.size());
   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()]);
 }
 
 TYPED_TEST(DenseMapTest, AssignmentTestNotSmall) {
   for (int Key = 0; Key < 5; ++Key)
     this->Map[this->getKey(Key)] = this->getValue(Key);
   TypeParam copyMap = this->Map;
 
   EXPECT_EQ(5u, copyMap.size());
   for (int Key = 0; Key < 5; ++Key)
     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)]);
 }
 
 // Test swap method
 TYPED_TEST(DenseMapTest, SwapTest) {
   this->Map[this->getKey()] = this->getValue();

llvm/trunk/unittests/ADT/SmallPtrSetTest.cpp

TEST(SmallPtrSetTest, Assignment) {
   for (int i = 0; i < 8; ++i)
     buf[i] = 0;
 
   SmallPtrSet<int *, 4> s1 = {&buf[0], &buf[1]};
   SmallPtrSet<int *, 4> s2;
   (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());
   for (int i = 0; i < 8; ++i)
     if (i < 4)
       EXPECT_TRUE(s1.count(&buf[i]));
     else
       EXPECT_FALSE(s1.count(&buf[i]));

 TEST(SmallPtrSetTest, GrowthTest) {
   int i;
   int buf[8];
   for(i=0; i<8; ++i) buf[i]=0;
 
 
   SmallPtrSet<int *, 4> s;
   typedef SmallPtrSet<int *, 4>::iterator iter;
-  
   s.insert(&buf[0]);
   s.insert(&buf[1]);
   s.insert(&buf[2]);
   s.insert(&buf[3]);
   EXPECT_EQ(4U, s.size());
 
   i = 0;
   for(iter I=s.begin(), E=s.end(); I!=E; ++I, ++i)

llvm/trunk/unittests/ADT/SparseBitVectorTest.cpp

TEST(SparseBitVectorTest, IntersectWith) {
   EXPECT_TRUE(Vec.empty());
 }
 
 TEST(SparseBitVectorTest, SelfAssignment) {
   SparseBitVector<> Vec, Other;
 
   Vec.set(23);
   Vec.set(234);
-  Vec = Vec;
+  Vec = static_cast<SparseBitVector<> &>(Vec);
   EXPECT_TRUE(Vec.test(23));
   EXPECT_TRUE(Vec.test(234));
 
   Vec.clear();
   Vec.set(17);
   Vec.set(256);
   EXPECT_FALSE(Vec |= Vec);
   EXPECT_TRUE(Vec.test(17));