[PATCH] D15456: [PATCH] New diagnostic for non-idiomatic copy or move operations (v2)

[Richard Smith via cfe-commits]

rsmith added a comment.

I'm unconvinced this meets the bar for an on-by-default warning. Your suggested workaround for correct-but-diagnosed code doesn't seem to work: if I have

  struct X {
    X(const volatile X&);
  };

... adding

  X(const X&) = delete;

or a private "normal" copy constructor will break clients of my code that happen to have a non-volatile `X` object. This will also diagnose the pre-C++11 idiom of:

  struct noncopyable {
    noncopyable(noncopyable&);
    void operator=(noncopyable&);
  };

... where the references are deliberately references to non-const in order to allow more bugs to be caught at compile time.

I'd also expect that if the user wrote more tokens than would be present in the idiomatic declaration, they probably know what they're doing (so don't reject extra cv-qualifiers and ref-qualifiers).

Can you provide a list of the things this found in Qt and rethinkdb? Is there some pattern in them? Are they bugs?


================
Comment at: lib/Sema/SemaDeclCXX.cpp:4924-4929
@@ +4923,8 @@
+                                           bool Move) {
+  // Check that there are no ref-qualifiers.
+  if (Assign->getRefQualifier() != RQ_None) {
+    S.Diag(Assign->getLocation(), diag::warn_non_idiomatic_copy_move_assign)
+        << Move << /*ref-qualifier*/ 2;
+    return;
+  }
+
----------------
I don't think it makes sense to warn on this when the qualifier is `&`. In that case, the user's code is *better* than the normal idiom. And if they used the ref-qualifier `&&`, perhaps we should just assume they know what they're doing?


http://reviews.llvm.org/D15456