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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
