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

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

(Sorry, fat fingered the original response.)

In http://reviews.llvm.org/D15456#320946, @rsmith wrote:

> 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.


That's a good point. You could use delegating constructors to silence the warning, but that is a bit intrusive (and non-functional for coding targeting older versions of the standard).

> 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.


This was intentional, based on discussion. The thought was that (1) these are still copy constructible/assignable (within the context of the class type itself), and (2) they're still not idiomatic. However, I do think that's a bit of a weak argument and would be fine allowing those cases.

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


It would be oddly inconsistent to warn on some non-idiomatic operations, but then fail to warn on other non-idiomatic operations though. However, it may make sense

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


The Qt issues are things like: http://code.qt.io/cgit/qt/qtbase.git/tree/src/tools/uic/ui4.h?h=dev#n379 However, there are also plenty like:

dom/QualifiedName.h:70:11: warning: non-idiomatic copy assignment operator declaration; consider returning 'T&' instead [-Wnon-idiomatic-copy-move]

  const QualifiedName& operator=(const QualifiedName& other) { other.ref(); deref(); m_impl = other.m_impl; return *this; }

Rethink's look more like:
./src/rapidjson/document.h:329:5: error: non-idiomatic copy assignment operator declaration; consider returning 'T&' instead [-Werror,-Wnon-idiomatic-copy-move]

  GenericStringRef operator=(const GenericStringRef&);
  ^

./src/rapidjson/document.h:595:43: error: non-idiomatic copy assignment operator declaration; consider 'const T&' instead [-Werror,-Wnon-idiomatic-copy-move]

  GenericValue& operator=(GenericValue& rhs) RAPIDJSON_NOEXCEPT {
                                        ^

Whether the issues pointed out are bugs or not is a bit more subjective as it depends on the usage. Some seems like they aren't (such as Qt's stuff in ui4.h which are old-style "deleted" functions). Others seem like they definitely are, such as what I pointed out from rethinkdb (though rethink also had some old-style deleted functions as well). And others seem questionable, such as Qt's QualifiedName, where it could go either way.


http://reviews.llvm.org/D15456