Bug 58407 - [C++11] Should warn about deprecated implicit generation of copy constructor/assignment
Summary: [C++11] Should warn about deprecated implicit generation of copy constructor/...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.8.1
: P3 enhancement
Target Milestone: 9.0
Assignee: Jason Merrill
URL:
Keywords: diagnostic, patch
Depends on:
Blocks: Weffc++
  Show dependency treegraph
 
Reported: 2013-09-12 18:48 UTC by Andrzej Krzemienski
Modified: 2018-10-02 19:35 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2014-04-27 00:00:00


Attachments
P (3.02 KB, text/plain)
2018-03-13 00:03 UTC, Jason Merrill
Details
updated patch (5.49 KB, patch)
2018-05-16 01:21 UTC, Jason Merrill
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrzej Krzemienski 2013-09-12 18:48:21 UTC
The following program is correct in C++11, but uses a deprecated language feature. According section D.3, paragraph 1:
"The implicit definition of a copy constructor as defaulted is deprecated if the class has a user-declared copy assignment operator or a user-declared destructor. The implicit definition of a copy assignment operator as defaulted is deprecated if the class has a user-declared copy constructor or a user-declared destructor." 

G++ should emit a warning in C++11 mode.

====================

struct W {
  int a;
  ~W() { a = 9; }
};

int main() {
 W w {};
 W v = w;
}
Comment 1 Paolo Carlini 2013-09-12 19:15:27 UTC
Out of curiosity, are you aware of other compilers emitting such warning? With which wording, in case? Because I don't seem to be able to get one from current clang and icc.
Comment 2 Andrzej Krzemienski 2013-09-12 19:27:07 UTC
No. Other compilers (Clang and VS 2010) do not emit such warning either.
Comment 3 Paolo Carlini 2013-09-12 19:37:39 UTC
I see. Have to double check our policy in this area, I'm not at all sure that in general anything in Section D gets a warning.
Comment 4 Jonathan Wakely 2013-09-12 19:48:31 UTC
I don't think anything in appendix D apart from auto_ptr does.

The warning would be useful to help find places where defaulted definitions could be added, but I'm not sure I'd want it enabled by -Wall. YMMV.
Comment 5 Daniel Krügler 2013-09-12 19:56:12 UTC
(In reply to Jonathan Wakely from comment #4)

-pedantic would seem like a good candidate, no?
Comment 6 Andrzej Krzemienski 2013-09-13 10:47:55 UTC
(In reply to Andrzej Krzemienski from comment #2)
> No. Other compilers (Clang and VS 2010) do not emit such warning either.

Correction: The newest version of Clang does give a warning:
"definition of implicit copy constructor for 'W' is deprecated because it has a user-declared destructor [-Wdeprecated]"
Comment 7 Andrzej Krzemienski 2014-04-17 08:50:56 UTC
Just one remark. A warning in this situation is not to just warn about any deprecated feature, but to indicate something that is very likely to be a bug (It used to be legal in C++03, so you couldn't legally warn about it; now the deprecation is just an additional incentive). My guess is that having this copy constructor implicitly defined when you have a custom destructor is a potential bug in half of the cases. (Well, "half" is my guess.)
Comment 8 Alex Howlett 2014-04-27 00:05:11 UTC
I found this thread because I went Googling after I discovered a bug in my code resulting from the copy operations being generated despite my having written a destructor.  I was angry that my compiler (Microsoft) hadn't emitted a warning.

Please add this warning if you haven't already.
Comment 9 Jonathan Wakely 2014-04-27 12:13:25 UTC
(In reply to Andrzej Krzemienski from comment #7)
> (It used to be legal in C++03, so you couldn't legally warn about it;

That's not  true, the compiler can (and does) warn about legal code.

I'm confirming this, we will want the warning at some point, and it would allow us to improve this part of the -Weffc++ warnings:

* Item 11: Define a copy constructor and an assignment operator for classes
with dynamically allocated memory.

(see PR 16166 for more details)

Maybe we could call this warning -Wdeprecated-special-members, and have it enabled -Weffc++ and in C++11 also by -Wdeprecated
Comment 10 Andrzej Krzemienski 2014-04-27 20:20:11 UTC
I guess, by now "item 11" should read "either delete or define ..."
Comment 11 Jason Merrill 2018-03-13 00:03:13 UTC
Created attachment 43637 [details]
P
Comment 12 Jason Merrill 2018-03-13 00:06:08 UTC
Here's an implementation to play with.  It breaks lots of tests in the library, which I'm not going to continue poking at now.  The deque warning in particular seems tricky to address.
Comment 13 Jonathan Wakely 2018-03-13 12:18:01 UTC
Thanks! I can take care of the rest of the library, but for now this makes all the deque tests pass:


--- a/libstdc++-v3/include/bits/stl_deque.h
+++ b/libstdc++-v3/include/bits/stl_deque.h
@@ -149,9 +149,23 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       _Deque_iterator() _GLIBCXX_NOEXCEPT
       : _M_cur(), _M_first(), _M_last(), _M_node() { }
 
+#if __cplusplus < 201103L
+      // Conversion from iterator to const_iterator.
       _Deque_iterator(const iterator& __x) _GLIBCXX_NOEXCEPT
       : _M_cur(__x._M_cur), _M_first(__x._M_first),
        _M_last(__x._M_last), _M_node(__x._M_node) { }
+#else
+      // Conversion from iterator to const_iterator.
+      template<typename _Iter,
+              typename = _Require<is_same<_Self, const_iterator>,
+                                  is_same<_Iter, iterator>>>
+       _Deque_iterator(const _Iter& __x) noexcept
+       : _M_cur(__x._M_cur), _M_first(__x._M_first),
+         _M_last(__x._M_last), _M_node(__x._M_node) { }
+
+      _Deque_iterator(const _Deque_iterator&) = default;
+      _Deque_iterator& operator=(const _Deque_iterator&) = default;
+#endif
 
       iterator
       _M_const_cast() const _GLIBCXX_NOEXCEPT
diff --git a/libstdc++-v3/include/ext/throw_allocator.h b/libstdc++-v3/include/ext/throw_allocator.h
index 5d9caa2bcae..f773a2d79ba 100644
--- a/libstdc++-v3/include/ext/throw_allocator.h
+++ b/libstdc++-v3/include/ext/throw_allocator.h
@@ -403,6 +403,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   struct condition_base
   {
     virtual ~condition_base() { };
+#if __cplusplus >= 201103L
+    condition_base() = default;
+    condition_base(const condition_base&) = default;
+    condition_base& operator=(const condition_base&) = default;
+#endif
   };
Comment 14 Jason Merrill 2018-05-16 01:21:53 UTC
Created attachment 44135 [details]
updated patch

Here's an updated implementation, which is limited to -Wall, so it doesn't affect the building or testing of the library.  I'm sure you will still want to adjust the library to avoid warnings when users try to build with -Wall.

On looking more closely at some of the places I was getting warnings in the compiler, I noticed that they often seemed to be actual errors.  For instance, if someone copies an aa_tree and then destroys one of the aa_trees, the other is left with a pointer to garbage.

I also realized that we want to avoid warning when the copy is elided, particularly given C++17 mandatory copy elision; if the prvalue directly initializes a variable, there's no problematic copying.

An interesting subcategory I noticed when I had the warning on by default is classes with a virtual destructor, such as the exception hierarchy.  The warning calls attention to potential slicing problems, and so I think we don't want to add defaulted copy ops; it's good for users to be encouraged to e.g. catch by reference.
Comment 15 Jason Merrill 2018-05-18 20:03:19 UTC
Author: jason
Date: Fri May 18 20:02:48 2018
New Revision: 260381

URL: https://gcc.gnu.org/viewcvs?rev=260381&root=gcc&view=rev
Log:
	PR c++/58407 - deprecated implicit copy ops.

gcc/c-family/
	* c.opt (Wdeprecated-copy): New flag.
gcc/cp/
	* call.c (build_over_call): Warn about deprecated trivial fns.
	* class.c (classtype_has_user_copy_or_dtor): New.
	(type_build_ctor_call): Check TREE_DEPRECATED.
	(type_build_dtor_call): Likewise.
	* decl2.c (cp_warn_deprecated_use): Move from tree.c.
	Add checks.  Return bool.  Handle -Wdeprecated-copy.
	(mark_used): Use it.
	* decl.c (grokdeclarator): Remove redundant checks.
	* typeck2.c (build_functional_cast): Likewise.
	* method.c (lazily_declare_fn): Mark deprecated copy ops.
	* init.c (build_aggr_init): Only set TREE_USED if there are
	side-effects.
libitm/
	* beginend.cc (save): Disable -Werror=deprecated-copy.

Added:
    trunk/gcc/testsuite/g++.dg/cpp0x/depr-copy1.C
Modified:
    trunk/gcc/c-family/ChangeLog
    trunk/gcc/c-family/c.opt
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/call.c
    trunk/gcc/cp/class.c
    trunk/gcc/cp/cp-tree.h
    trunk/gcc/cp/decl.c
    trunk/gcc/cp/decl2.c
    trunk/gcc/cp/init.c
    trunk/gcc/cp/method.c
    trunk/gcc/cp/tree.c
    trunk/gcc/cp/typeck2.c
    trunk/gcc/doc/invoke.texi
    trunk/gcc/testsuite/g++.old-deja/g++.other/warn6.C
    trunk/libitm/ChangeLog
    trunk/libitm/beginend.cc
Comment 16 Jason Merrill 2018-05-18 23:43:52 UTC
(In reply to Jason Merrill from comment #14)
> An interesting subcategory I noticed when I had the warning on by default is
> classes with a virtual destructor, such as the exception hierarchy.  The
> warning calls attention to potential slicing problems, and so I think we
> don't want to add defaulted copy ops; it's good for users to be encouraged
> to e.g. catch by reference.

For the record, I changed my mind about this; a slicing warning should only apply to copies from an object of unknown dynamic type.

-Wdeprecated-copy added for GCC 9.
Comment 17 Steinar H. Gunderson 2018-07-30 13:25:22 UTC
Hi,

Does this change mean now it's effectively impossible to have abstract base classes under -Wall without adding boilerplate? Consider something like the following:

class Base {
public:
  virtual ~Base() {}
  virtual void foo() = 0;
};

class Derived : Base {
public:
  ~Derived();
  void foo() override;
};

Base needs to have a virtual destructor since it has virtual member functions (or half the world will give you warnings).

Any attempts now to copy Derived through the implicit copy constructor will give a warning, since the synthesis of Base::Base(const Base &) is deprecated. The only way I've found to suppress this is to add

  Base::Base(const Base &) = default;

However, this in turn disables the synthesis of Base::Base(), and also Base::operator=(const Base &). So I need:

  Base() = default;
  Base(const Base &) = default;
  Base(Base &&) = default;
  Base &operator= (const Base &) = default;
  Base &operator= (Base &&) = default;

for something that doesn't have a single member!

Am I missing something?
Comment 18 Jonathan Wakely 2018-07-30 14:19:41 UTC
(In reply to Steinar H. Gunderson from comment #17)
> Base needs to have a virtual destructor since it has virtual member
> functions (or half the world will give you warnings).

Or a protected destructor, but that doesn't change anything relevant to your problem.

> Any attempts now to copy Derived through the implicit copy constructor will
> give a warning, since the synthesis of Base::Base(const Base &) is
> deprecated. The only way I've found to suppress this is to add
> 
>   Base::Base(const Base &) = default;
> 
> However, this in turn disables the synthesis of Base::Base(), and also
> Base::operator=(const Base &). So I need:
> 
>   Base() = default;
>   Base(const Base &) = default;
>   Base(Base &&) = default;
>   Base &operator= (const Base &) = default;
>   Base &operator= (Base &&) = default;
> 
> for something that doesn't have a single member!

Yes. The deprecation means a future version of C++ might make your type non-copyable. To explicitly say it's copyable (and make the code futureproof) you need to add those defaulted members.

If you don't want to do that, you can use -Wno-deprecated-copy to suppress the warnings.
Comment 19 Steinar H. Gunderson 2018-07-30 14:23:21 UTC
Thanks for confirming; so GCC is absolutely right here, it's the standard that makes a choice with surprising ramifications (to me, at least). I wonder if I should try to ask someone in the standards committee to make an exception for pure virtual destructors...
Comment 20 Jonathan Wakely 2018-07-30 14:35:48 UTC
(In reply to Steinar H. Gunderson from comment #19)
> Thanks for confirming; so GCC is absolutely right here, it's the standard
> that makes a choice with surprising ramifications (to me, at least). I

Yes. GCC could choose not to warn about that deprecation, but the whole point of this bug report is that somebody was asking for GCC to do exactly that.

> wonder if I should try to ask someone in the standards committee to make an
> exception for pure virtual destructors...

I've heard worse ideas. The whole topic of deprecating those members is already a bit contentious.
Comment 21 Jonathan Wakely 2018-07-30 14:40:26 UTC
P.S. it's arguable whether abstract base classes should be copyable in the first place:
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rc-copy-virtual
https://github.com/isocpp/CppCoreGuidelines/issues/1151
Comment 22 Steinar H. Gunderson 2018-07-30 14:46:25 UTC
C.67 would seem only to apply to non-abstract base classes, no? The code doesn't compile if B has pure virtual member functions. (Well, it doesn't compile as-is already, but change (d) to (*d) and it does.)
Comment 23 Jonathan Wakely 2018-07-30 22:31:01 UTC
Right, sorry, I added that last comment too hastily.
Comment 24 Allan Jensen 2018-10-02 15:47:51 UTC
So with this the rule-of-three is now the rule-of-four or six?
Comment 25 Andrzej Krzemienski 2018-10-02 16:07:50 UTC
(In reply to Allan Jensen from comment #24)
> So with this the rule-of-three is now the rule-of-four or six?

The Rule of Zero: https://blog.rmf.io/cxx11/rule-of-zero
Comment 26 Steinar H. Gunderson 2018-10-02 16:28:48 UTC
That blog post seems to advocate using std::unique_ptr for pretty much everything, which unfortunately doesn't always work. See e.g. slide 8 of https://github.com/CppCon/CppCon2018/blob/master/Presentations/woes_of_scope_guards_and_unique_resource/woes_of_scope_guards_and_unique_resource__peter_sommerlad__cppcon_2018.pdf -- C++20 will have std::unique_resource, but we're not there yet.

Straying off course for the bug, though...