From ff59df6be2cf00317112038bb8606ab177a82e34 Mon Sep 17 00:00:00 2001 From: bgloyer <36457894+bgloyer@users.noreply.github.com> Date: Fri, 8 Oct 2021 23:34:55 -0700 Subject: [PATCH 01/14] Update CppCoreGuidelines.md --- CppCoreGuidelines.md | 54 +++++++++++++++++--------------------------- 1 file changed, 21 insertions(+), 33 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 33bda3fc5..297fb64de 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -6163,8 +6163,7 @@ After `y = std::move(x)` the value of `y` should be the value `x` had and `x` sh int sz; }; - - X::X(X&& a) + X::X(X&& a) noexcept :p{a.p}, sz{a.sz} // steal representation { a.p = nullptr; // set to "empty" @@ -6200,45 +6199,34 @@ Unless there is an exceptionally strong reason not to, make `x = std::move(y); y ##### Reason -If `x = x` changes the value of `x`, people will be surprised and bad errors can occur. However, people don't usually directly write a self-assignment that turn into a move, but it can occur. However, `std::swap` is implemented using move operations so if you accidentally do `swap(a, b)` where `a` and `b` refer to the same object, failing to handle self-move could be a serious and subtle error. +If `x = std::move(x)` changes the value of `x`, people will be surprised and bad errors can occur. However, people don't usually directly write a self-assignment that turn into a move, but it can occur. However, `std::swap` is implemented using move operations so if you accidentally do `swap(a, b)` where `a` and `b` refer to the same object, failing to handle self-move could be a serious and subtle error. ##### Example - class Foo { - string s; - int i; - public: - Foo& operator=(Foo&& a); - // ... - }; - - Foo& Foo::operator=(Foo&& a) noexcept // OK, but there is a cost - { - if (this == &a) return *this; // this line is redundant - s = std::move(a.s); - i = a.i; - return *this; - } - -The one-in-a-million argument against `if (this == &a) return *this;` tests from the discussion of [self-assignment](#Rc-copy-self) is even more relevant for self-move. +class X { +public: + X(); + X& operator=(X&& a) noexcept; + // ... + ~X() { delete[] owning_ptr; } +private: + int* owning_ptr; // bad, but just for example. See R.20 +}; -##### Note +X& X::operator=(X&& a) noexcept +{ + auto* temp = a.owning_ptr; + a.owning_ptr = nullptr; + delete owning_ptr; + owning_ptr = temp; + return *this; +} -There is no known general way of avoiding an `if (this == &a) return *this;` test for a move assignment and still get a correct answer (i.e., after `x = x` the value of `x` is unchanged). +The argument from the discussion of [self-assignment](#Rc-copy-self) against a `if (this == &a) return *this;` test is even more relevant for self-move. ##### Note -The ISO standard guarantees only a "valid but unspecified" state for the standard-library containers. Apparently this has not been a problem in about 10 years of experimental and production use. Please contact the editors if you find a counter example. The rule here is more caution and insists on complete safety. - -##### Example - -Here is a way to move a pointer without a test (imagine it as code in the implementation a move assignment): - - // move from other.ptr to this->ptr - T* temp = other.ptr; - other.ptr = nullptr; - delete ptr; - ptr = temp; +The default generated move assignment operator will correctly handle self-assignment if all members correctly handle self-assignment. ##### Enforcement From f211f4e99d8bc7b603e4ee2ed819df317a2f13db Mon Sep 17 00:00:00 2001 From: bgloyer <36457894+bgloyer@users.noreply.github.com> Date: Sun, 10 Oct 2021 22:13:14 -0700 Subject: [PATCH 02/14] re-wording --- CppCoreGuidelines.md | 42 ++++++++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 297fb64de..14edb1460 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -6203,24 +6203,30 @@ If `x = std::move(x)` changes the value of `x`, people will be surprised and bad ##### Example -class X { -public: - X(); - X& operator=(X&& a) noexcept; - // ... - ~X() { delete[] owning_ptr; } -private: - int* owning_ptr; // bad, but just for example. See R.20 -}; - -X& X::operator=(X&& a) noexcept -{ - auto* temp = a.owning_ptr; - a.owning_ptr = nullptr; - delete owning_ptr; - owning_ptr = temp; - return *this; -} +If all of the members of a type are safe for move assignment, the default generated move assignment operator will be safe too. + + Foo& operator=(Foo&& a) = default; + +Otherwise, the manually written move assignment operate must be made safe for self-assignement. + + class X { + public: + X(); + X& operator=(X&& a) noexcept; + // ... + ~X() { delete[] owning_ptr; } + private: + int* owning_ptr; // bad, but just for example. See R.20 + }; + + X& X::operator=(X&& a) noexcept + { + auto* temp = a.owning_ptr; + a.owning_ptr = nullptr; + delete owning_ptr; + owning_ptr = temp; + return *this; + } The argument from the discussion of [self-assignment](#Rc-copy-self) against a `if (this == &a) return *this;` test is even more relevant for self-move. From 0a5fa48234728c016e56b015fc6bd140dfe3013c Mon Sep 17 00:00:00 2001 From: bgloyer <36457894+bgloyer@users.noreply.github.com> Date: Sat, 16 Jul 2022 13:41:35 -0700 Subject: [PATCH 03/14] c.65 clean-up --- CppCoreGuidelines.md | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 667f32bc7..d291a603d 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -6214,13 +6214,18 @@ Unless there is an exceptionally strong reason not to, make `x = std::move(y); y ##### Reason -If `x = std::move(x)` changes the value of `x`, people will be surprised and bad errors can occur. However, people don't usually directly write a self-assignment that turn into a move, but it can occur. However, `std::swap` is implemented using move operations so if you accidentally do `swap(a, b)` where `a` and `b` refer to the same object, failing to handle self-move could be a serious and subtle error. +People don't usually directly write a self move assignment, but it can occur. `std::swap` is implemented using +move operations so if you accidentally do `swap(a, b)` where `a` and `b` refer to the same object, failing to +handle self-move could be a serious and subtle error. At a minimum, a self move should put the object into a +valid but unspecified state which is the same guarantee provided by the standard library containers. It is +not requred to leave the object unchanged. ##### Example -If all of the members of a type are safe for move assignment, the default generated move assignment operator will be safe too. +If all of the members of a type are safe for move assignment, the default generated move +assignment operator will be safe too. - Foo& operator=(Foo&& a) = default; + X& operator=(X&& a) = default; Otherwise, the manually written move assignment operate must be made safe for self-assignement. @@ -6231,24 +6236,18 @@ Otherwise, the manually written move assignment operate must be made safe for se // ... ~X() { delete[] owning_ptr; } private: - int* owning_ptr; // bad, but just for example. See R.20 + T* owning_ptr; // bad, but just for example. See R.20 }; X& X::operator=(X&& a) noexcept { auto* temp = a.owning_ptr; a.owning_ptr = nullptr; - delete owning_ptr; + delete owning_ptr; // null in the case of a self move owning_ptr = temp; return *this; } -The argument from the discussion of [self-assignment](#Rc-copy-self) against a `if (this == &a) return *this;` test is even more relevant for self-move. - -##### Note - -The default generated move assignment operator will correctly handle self-assignment if all members correctly handle self-assignment. - ##### Enforcement * (Moderate) In the case of self-assignment, a move assignment operator should not leave the object holding pointer members that have been `delete`d or set to `nullptr`. From 67da2730db2a2cdb9ee69512afe5dbb755aac852 Mon Sep 17 00:00:00 2001 From: bgloyer <36457894+bgloyer@users.noreply.github.com> Date: Sat, 16 Jul 2022 16:31:47 -0700 Subject: [PATCH 04/14] wording clean-up --- CppCoreGuidelines.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index d291a603d..2859a2d56 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -6216,7 +6216,7 @@ Unless there is an exceptionally strong reason not to, make `x = std::move(y); y People don't usually directly write a self move assignment, but it can occur. `std::swap` is implemented using move operations so if you accidentally do `swap(a, b)` where `a` and `b` refer to the same object, failing to -handle self-move could be a serious and subtle error. At a minimum, a self move should put the object into a +handle self-move could be a serious and subtle error. At a minimum, a self-move should put the object into a valid but unspecified state which is the same guarantee provided by the standard library containers. It is not requred to leave the object unchanged. @@ -6227,7 +6227,7 @@ assignment operator will be safe too. X& operator=(X&& a) = default; -Otherwise, the manually written move assignment operate must be made safe for self-assignement. +Otherwise, the manually written move-assignment operater must be made safe for self-assignement. class X { public: @@ -6236,7 +6236,8 @@ Otherwise, the manually written move assignment operate must be made safe for se // ... ~X() { delete[] owning_ptr; } private: - T* owning_ptr; // bad, but just for example. See R.20 + T* owning_ptr; // bad (See R.20) but used in the example because + // it requires a manual move assignment }; X& X::operator=(X&& a) noexcept @@ -6250,7 +6251,7 @@ Otherwise, the manually written move assignment operate must be made safe for se ##### Enforcement -* (Moderate) In the case of self-assignment, a move assignment operator should not leave the object holding pointer members that have been `delete`d or set to `nullptr`. +* (Moderate) In the case of self-assignment, a move assignment operator should not leave the object holding pointer members that have been `delete`d. * (Not enforceable) Look at the use of standard-library container types (incl. `string`) and consider them safe for ordinary (not life-critical) uses. ### C.66: Make move operations `noexcept` From 1a5c2699fac6674c358dba7278d8499e34057b43 Mon Sep 17 00:00:00 2001 From: bgloyer <36457894+bgloyer@users.noreply.github.com> Date: Sat, 16 Jul 2022 16:36:38 -0700 Subject: [PATCH 05/14] clean-up --- CppCoreGuidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 2859a2d56..35ffbf219 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -6251,7 +6251,7 @@ Otherwise, the manually written move-assignment operater must be made safe for s ##### Enforcement -* (Moderate) In the case of self-assignment, a move assignment operator should not leave the object holding pointer members that have been `delete`d. +* (Moderate) In the case of self-assignment, a move assignment operator should not leave the object holding pointer members that have been deleted. * (Not enforceable) Look at the use of standard-library container types (incl. `string`) and consider them safe for ordinary (not life-critical) uses. ### C.66: Make move operations `noexcept` From 63cf4d76799a3881683a614c2dedf90c499a2623 Mon Sep 17 00:00:00 2001 From: bgloyer <36457894+bgloyer@users.noreply.github.com> Date: Sat, 16 Jul 2022 16:40:48 -0700 Subject: [PATCH 06/14] made hyphens more consistent --- CppCoreGuidelines.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 35ffbf219..f03333d18 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -6214,7 +6214,7 @@ Unless there is an exceptionally strong reason not to, make `x = std::move(y); y ##### Reason -People don't usually directly write a self move assignment, but it can occur. `std::swap` is implemented using +People don't usually directly write a self-move assignment, but it can occur. `std::swap` is implemented using move operations so if you accidentally do `swap(a, b)` where `a` and `b` refer to the same object, failing to handle self-move could be a serious and subtle error. At a minimum, a self-move should put the object into a valid but unspecified state which is the same guarantee provided by the standard library containers. It is @@ -6222,8 +6222,8 @@ not requred to leave the object unchanged. ##### Example -If all of the members of a type are safe for move assignment, the default generated move -assignment operator will be safe too. +If all of the members of a type are safe for move-assignment, the default generated move-assignment +operator will be safe too. X& operator=(X&& a) = default; @@ -6237,7 +6237,7 @@ Otherwise, the manually written move-assignment operater must be made safe for s ~X() { delete[] owning_ptr; } private: T* owning_ptr; // bad (See R.20) but used in the example because - // it requires a manual move assignment + // it requires a manual move-assignment }; X& X::operator=(X&& a) noexcept From 3863635e6e7e63607adfc6e2bedaf40743385645 Mon Sep 17 00:00:00 2001 From: bgloyer <36457894+bgloyer@users.noreply.github.com> Date: Sat, 16 Jul 2022 17:37:00 -0700 Subject: [PATCH 07/14] fix delete --- CppCoreGuidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index f03333d18..bd02d6cd9 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -6234,7 +6234,7 @@ Otherwise, the manually written move-assignment operater must be made safe for s X(); X& operator=(X&& a) noexcept; // ... - ~X() { delete[] owning_ptr; } + ~X() { delete owning_ptr; } private: T* owning_ptr; // bad (See R.20) but used in the example because // it requires a manual move-assignment From 839c4df2e7fdd6782c9caff99aed27d0e26d0aa2 Mon Sep 17 00:00:00 2001 From: bgloyer <36457894+bgloyer@users.noreply.github.com> Date: Sat, 16 Jul 2022 18:01:34 -0700 Subject: [PATCH 08/14] spelling --- CppCoreGuidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index bd02d6cd9..e1fede360 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -6227,7 +6227,7 @@ operator will be safe too. X& operator=(X&& a) = default; -Otherwise, the manually written move-assignment operater must be made safe for self-assignement. +Otherwise, the manually written move-assignment operator must be made safe for self-assignement. class X { public: From a3ccaa3814c3063158d9dfbce37ea03e43a2f4ab Mon Sep 17 00:00:00 2001 From: bgloyer <36457894+bgloyer@users.noreply.github.com> Date: Sat, 16 Jul 2022 18:22:28 -0700 Subject: [PATCH 09/14] revert C.64 change --- CppCoreGuidelines.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index e1fede360..bc78d66d5 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -6178,7 +6178,8 @@ After `y = std::move(x)` the value of `y` should be the value `x` had and `x` sh int sz; }; - X::X(X&& a) noexcept + + X::X(X&& a) :p{a.p}, sz{a.sz} // steal representation { a.p = nullptr; // set to "empty" From a9ac67a4c605466075ed72ce486b91297a291185 Mon Sep 17 00:00:00 2001 From: bgloyer <36457894+bgloyer@users.noreply.github.com> Date: Sat, 16 Jul 2022 18:37:24 -0700 Subject: [PATCH 10/14] reused m_owning --- CppCoreGuidelines.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index bc78d66d5..d619d3652 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -6235,18 +6235,18 @@ Otherwise, the manually written move-assignment operator must be made safe for s X(); X& operator=(X&& a) noexcept; // ... - ~X() { delete owning_ptr; } + ~X() { delete m_owning; } private: - T* owning_ptr; // bad (See R.20) but used in the example because - // it requires a manual move-assignment + T* m_owning; // bad (See R.20) but used in the example because + // it requires a manual move-assignment }; X& X::operator=(X&& a) noexcept { - auto* temp = a.owning_ptr; - a.owning_ptr = nullptr; - delete owning_ptr; // null in the case of a self move - owning_ptr = temp; + auto* temp = a.m_owning; + a.m_owning = nullptr; + delete m_owning; // null in the case of a self move + m_owning = temp; return *this; } From 4ece32b280fbdb4e265750c0eb655b4374e045f8 Mon Sep 17 00:00:00 2001 From: bgloyer <36457894+bgloyer@users.noreply.github.com> Date: Sat, 16 Jul 2022 19:00:26 -0700 Subject: [PATCH 11/14] spelling --- CppCoreGuidelines.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index d619d3652..12e6babc7 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -6219,7 +6219,7 @@ People don't usually directly write a self-move assignment, but it can occur. ` move operations so if you accidentally do `swap(a, b)` where `a` and `b` refer to the same object, failing to handle self-move could be a serious and subtle error. At a minimum, a self-move should put the object into a valid but unspecified state which is the same guarantee provided by the standard library containers. It is -not requred to leave the object unchanged. +not required to leave the object unchanged. ##### Example @@ -6228,7 +6228,7 @@ operator will be safe too. X& operator=(X&& a) = default; -Otherwise, the manually written move-assignment operator must be made safe for self-assignement. +Otherwise, the manually written move-assignment operator must be made safe for self-assignment class X { public: From eec4728dd4939436f63c9688e3f3207ee21aaa45 Mon Sep 17 00:00:00 2001 From: bgloyer <36457894+bgloyer@users.noreply.github.com> Date: Sat, 16 Jul 2022 19:06:40 -0700 Subject: [PATCH 12/14] temp -> tmp --- CppCoreGuidelines.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 12e6babc7..6f642d539 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -6243,10 +6243,10 @@ Otherwise, the manually written move-assignment operator must be made safe for s X& X::operator=(X&& a) noexcept { - auto* temp = a.m_owning; + auto* tmp = a.m_owning; a.m_owning = nullptr; delete m_owning; // null in the case of a self move - m_owning = temp; + m_owning = tmp; return *this; } From b60a5db5739c0e5b5a9d9d9c18890c65c77858e5 Mon Sep 17 00:00:00 2001 From: bgloyer <36457894+bgloyer@users.noreply.github.com> Date: Sat, 16 Jul 2022 19:13:02 -0700 Subject: [PATCH 13/14] incl. -> include --- CppCoreGuidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 6f642d539..6963503fc 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -6253,7 +6253,7 @@ Otherwise, the manually written move-assignment operator must be made safe for s ##### Enforcement * (Moderate) In the case of self-assignment, a move assignment operator should not leave the object holding pointer members that have been deleted. -* (Not enforceable) Look at the use of standard-library container types (incl. `string`) and consider them safe for ordinary (not life-critical) uses. +* (Not enforceable) Look at the use of standard-library container types (including `string`) and consider them safe for ordinary (not life-critical) uses. ### C.66: Make move operations `noexcept` From 31d1433d6bb40b2df110e21c611dc273e29fa838 Mon Sep 17 00:00:00 2001 From: bgloyer <36457894+bgloyer@users.noreply.github.com> Date: Sun, 17 Jul 2022 22:29:57 -0700 Subject: [PATCH 14/14] remove trailing space --- CppCoreGuidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 6963503fc..ba881ab71 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -6238,7 +6238,7 @@ Otherwise, the manually written move-assignment operator must be made safe for s ~X() { delete m_owning; } private: T* m_owning; // bad (See R.20) but used in the example because - // it requires a manual move-assignment + // it requires a manual move-assignment }; X& X::operator=(X&& a) noexcept