<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jan 7, 2016 at 8:00 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron.ballman@gmail.com" target="_blank">aaron.ballman@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">aaron.ballman added a comment.<br>
<br>
(Sorry, fat fingered the original response.)<br>
<span class=""><br>
In <a href="http://reviews.llvm.org/D15456#320946" rel="noreferrer" target="_blank">http://reviews.llvm.org/D15456#320946</a>, @rsmith wrote:<br>
<br>
> 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<br>
><br>
>   struct X {<br>
>     X(const volatile X&);<br>
>   };<br>
><br>
><br>
> ... adding<br>
><br>
>   X(const X&) = delete;<br>
><br>
><br>
> or a private "normal" copy constructor will break clients of my code that happen to have a non-volatile `X` object.<br>
<br>
<br>
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).<br>
<br>
> This will also diagnose the pre-C++11 idiom of:<br>
<br>
><br>
<br>
>   struct noncopyable {<br>
<br>
>     noncopyable(noncopyable&);<br>
<br>
>     void operator=(noncopyable&);<br>
<br>
>   };<br>
<br>
><br>
<br>
><br>
<br>
> ... where the references are deliberately references to non-const in order to allow more bugs to be caught at compile time.<br>
<br>
<br>
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.<br>
<br>
> 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).<br>
<br>
<br>
It would be oddly inconsistent to warn on some non-idiomatic operations, but then fail to warn on other non-idiomatic operations though.</span></blockquote><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""> However, it may make sense<br>
<br>
> Can you provide a list of the things this found in Qt and rethinkdb? Is there some pattern in them? Are they bugs?<br>
<br>
<br>
The Qt issues are things like: <a href="http://code.qt.io/cgit/qt/qtbase.git/tree/src/tools/uic/ui4.h?h=dev#n379" rel="noreferrer" target="_blank">http://code.qt.io/cgit/qt/qtbase.git/tree/src/tools/uic/ui4.h?h=dev#n379</a></span></blockquote><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">However, there are also plenty like:<br>
<br>
dom/QualifiedName.h:70:11: warning: non-idiomatic copy assignment operator declaration; consider returning 'T&' instead [-Wnon-idiomatic-copy-move]<br>
<br>
  const QualifiedName& operator=(const QualifiedName& other) { other.ref(); deref(); m_impl = other.m_impl; return *this; }<br>
<br>
Rethink's look more like:<br>
</span>./src/rapidjson/document.h:329:5: error: non-idiomatic copy assignment operator declaration; consider returning 'T&' instead [-Werror,-Wnon-idiomatic-copy-move]<br>
<br>
  GenericStringRef operator=(const GenericStringRef&);<br>
  ^<br></blockquote><div><br></div><div>I'm happy for us to warn on both of these cases by default.</div><div><br></div><div>(But I don't think we should warn on a case like this:</div><div><br></div><div>  const T &operator=(const T &) const &;</div><div><br></div><div>... 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.)</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
./src/rapidjson/document.h:595:43: error: non-idiomatic copy assignment operator declaration; consider 'const T&' instead [-Werror,-Wnon-idiomatic-copy-move]<br>
<br>
  GenericValue& operator=(GenericValue& rhs) RAPIDJSON_NOEXCEPT {<br>
                                        ^<br></blockquote><div><br></div><div>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.</div></div></div></div>