<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Feb 3, 2016 at 1:03 PM, Jonathan Coe <span dir="ltr"><<a href="mailto:jon@jbcoe.net" target="_blank">jon@jbcoe.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On 3 February 2016 at 18:44, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Wed, Feb 3, 2016 at 10:23 AM, Jonathan Coe <span dir="ltr"><<a href="mailto:jon@jbcoe.net" target="_blank">jon@jbcoe.net</a>></span> wrote:<br><blockquote class="gmail_quote"><div dir="ltr">All the C++ compilers I have tried using (GCC,Clang,MSVC) will generate assignment operators even if the user defines a copy-constructor. This is the behaviour I set out to write a check for.<div><br></div><div>The cpp core guide lines recommend defining all or none of the special functions <a href="https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c21-if-you-define-or-delete-any-default-operation-define-or-delete-them-all" target="_blank">https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c21-if-you-define-or-delete-any-default-operation-define-or-delete-them-all</a></div><div><br></div><div>If the deprecation warning you mention will turn off the deprecated generation of special member functions when others are defined (or warn when deprecated compiler-generated functions are used) then the rule-of-five check is of reduced value.</div></div></blockquote><div><br></div></span><div>It can't stop them being generated, because that's required behavior - warnings don't change the program behavior.<br><br></div></div></div></div></blockquote><div><br></div></span><div>That's true but promoting them to errors will stop compilation and prevent the sort of bug I'm trying to stop.</div></div></div></div></blockquote><div><br></div><div>Sure - and the user can do that with -Werror=deprecated (but, yes, we should split out that specific deprecation so it can be turned on without turning on other deprecation)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Wording like this is in the C++11 standard:<br><br>"If the class definition does not explicitly declare a copy constructor, one is declared implicitly. If the class
definition declares a move constructor or move assignment operator, the implicitly declared copy constructor
is defined as deleted; otherwise, it is defined as defaulted (8.4). The latter case is deprecated if the class has
a user-declared copy assignment operator or a user-declared destructor."<br><br></div></div></div></div></blockquote><div><br></div></span><div>The 'deprecated' part is my target. I've seen code with user-defined copy constructors behave badly when compiler-generated assignment operators are unwittingly used. </div></div></div></div></blockquote><div><br></div><div>For sure - because it's only deprecated, not an error. Clang-tidy won't change that - it still won't be a hard error in every codebase.<br><br>I agree that having all the Core Guidelines checks in one place is a good idea, so I'm not making any real suggestion here, sorry - just that it seems unfortunate to relegate this check (& encourage explicit & mostly redundant defaulting/deleting) to a separate tool when it's essentially built into the language and compiler already. My disagreement is perhaps more with the Core Guideline than with its implementation here.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>The rule of five lets me locally reason about code without having to worry (needlessly or not) about whether some functions are potentially being compiler-generated.</div></div></div></div></blockquote><div><br></div><div>But once the language does the right thing directly, rather than providing a clang-tidy warning that encourages you to change the code to achieve the same result, why would we worry about being explicit?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>I don't advocate putting this in 'misc'. It belongs in 'cppcoreguidelines', I'll move it if the approach taken thus far is sound (as discussed in the review).</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>(similar wording for the copy assignment operator, the dtor has a different/special case if I recall correctly)</div><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><br></div><div>Jon</div></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On 3 February 2016 at 17:40, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Is this really that useful of a rule? The language does the right thing for the most part already (you don't need to explicitly delete them - they're implicitly deleted if you define any others - except for backcompat with C++98, but those cases are deprecated & we should probably split out the warning for that deprecation so it's easier to turn on separately)</div><div class="gmail_extra"><br><div class="gmail_quote"><div><div>On Wed, Feb 3, 2016 at 5:25 AM, Jonathan B Coe via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br></div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div>jbcoe retitled this revision from "clang-tidy check: Assignment and Construction" to "clang-tidy check: rule-of-five".<br>
jbcoe removed rL LLVM as the repository for this revision.<br>
jbcoe updated this revision to Diff 46775.<br>
jbcoe added a comment.<br>
<br>
I've responded to review comments (thanks for those) and renamed the check to 'rule-of-five'.<br>
<br>
I think it needs moving to cppcoreguidelines and needs destructor handling adding to it. As suggested, I'll address that in a later patch if everything else looks ok.<br>
<br>
<br>
<a href="http://reviews.llvm.org/D16376" rel="noreferrer" target="_blank">http://reviews.llvm.org/D16376</a><br>
<br>
Files:<br>
clang-tidy/misc/CMakeLists.txt<br>
clang-tidy/misc/MiscTidyModule.cpp<br>
clang-tidy/misc/RuleOfFiveCheck.cpp<br>
clang-tidy/misc/RuleOfFiveCheck.h<br>
docs/clang-tidy/checks/list.rst<br>
docs/clang-tidy/checks/misc-rule-of-five.rst<br>
test/clang-tidy/misc-rule-of-five.cpp<br>
<br>
<br></div></div>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div>
</blockquote></div><br></div>
</div></div></blockquote></span></div><br></div></div>
</blockquote></span></div><br></div></div>
</blockquote></div><br></div></div>