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

[Richard Smith via cfe-commits]

On Thu, Jan 7, 2016 at 8:00 AM, Aaron Ballman <aaron.ballman at gmail.com>
wrote:

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


I think it's overstepping our bounds to warn on merely non-idiomatic code
(that's more in the realm of clang-tidy). But warning on code that's
non-idiomatic /and/ probably not what the author intended seems reasonable.

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


Using 'void' for private unimplemented operator= functions is idiomatic for
some. I think we should suppress the warning if the function is private or
deleted. And honestly, given how rare it is to use the result of an
assignment, perhaps we should allow the return type to be 'void' in all
cases? Returning 'void' from your operator= doesn't seem like an
obviously-bad choice.

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&);
>   ^
>

I'm happy for us to warn on both of these cases by default.

(But I don't think we should warn on a case like this:

  const T &operator=(const T &) const &;

... because the user is clearly doing something weird (probably a DSL of
some kind) and the non-idiomatic assignment is very likely what they
wanted.)

./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 {
>                                         ^
>

I've dug up the relevant code, and that one is performing move emulation. I
don't think we're doing anyone any favours by warning on it by default,
because the code *does* mean exactly what the author wanted it to mean. A
clang-tidy check saying to use GenericValue&& seems very reasonable for
this code.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160107/29c3e8c1/attachment-0001.html>