On Wed, Feb 3, 2016 at 1:03 PM, Jonathan Coe <j...@jbcoe.net> wrote: > > > On 3 February 2016 at 18:44, David Blaikie <dblai...@gmail.com> wrote: > >> >> >> On Wed, Feb 3, 2016 at 10:23 AM, Jonathan Coe <j...@jbcoe.net> wrote: >> >>> 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. >>> >>> The cpp core guide lines recommend defining all or none of the special >>> functions >>> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c21-if-you-define-or-delete-any-default-operation-define-or-delete-them-all >>> >>> 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. >>> >> >> It can't stop them being generated, because that's required behavior - >> warnings don't change the program behavior. >> >> > That's true but promoting them to errors will stop compilation and prevent > the sort of bug I'm trying to stop. >
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) > > >> Wording like this is in the C++11 standard: >> >> "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." >> >> > 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. > 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. 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. > 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. > 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? > > 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). > > >> (similar wording for the copy assignment operator, the dtor has a >> different/special case if I recall correctly) >> >> >>> >>> Jon >>> >>> On 3 February 2016 at 17:40, David Blaikie <dblai...@gmail.com> wrote: >>> >>>> 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) >>>> >>>> On Wed, Feb 3, 2016 at 5:25 AM, Jonathan B Coe via cfe-commits < >>>> cfe-commits@lists.llvm.org> wrote: >>>> >>>>> jbcoe retitled this revision from "clang-tidy check: Assignment and >>>>> Construction" to "clang-tidy check: rule-of-five". >>>>> jbcoe removed rL LLVM as the repository for this revision. >>>>> jbcoe updated this revision to Diff 46775. >>>>> jbcoe added a comment. >>>>> >>>>> I've responded to review comments (thanks for those) and renamed the >>>>> check to 'rule-of-five'. >>>>> >>>>> 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. >>>>> >>>>> >>>>> http://reviews.llvm.org/D16376 >>>>> >>>>> Files: >>>>> clang-tidy/misc/CMakeLists.txt >>>>> clang-tidy/misc/MiscTidyModule.cpp >>>>> clang-tidy/misc/RuleOfFiveCheck.cpp >>>>> clang-tidy/misc/RuleOfFiveCheck.h >>>>> docs/clang-tidy/checks/list.rst >>>>> docs/clang-tidy/checks/misc-rule-of-five.rst >>>>> test/clang-tidy/misc-rule-of-five.cpp >>>>> >>>>> >>>>> _______________________________________________ >>>>> cfe-commits mailing list >>>>> cfe-commits@lists.llvm.org >>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>>>> >>>>> >>>> >>> >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
