From 2613ac4838954d408f590c6d9ce3aba09877e1c0 Mon Sep 17 00:00:00 2001 From: Tomasz Kaminski Date: Wed, 8 May 2024 16:22:18 +0200 Subject: [PATCH 1/7] CPP-5096 S1236 Explain covered cases in rspec --- rules/S1236/cfamily/metadata.json | 2 +- rules/S1236/cfamily/rule.adoc | 177 +++++++++++++++++++++++++++--- 2 files changed, 161 insertions(+), 18 deletions(-) diff --git a/rules/S1236/cfamily/metadata.json b/rules/S1236/cfamily/metadata.json index 522863a342d..8ae76d788c8 100644 --- a/rules/S1236/cfamily/metadata.json +++ b/rules/S1236/cfamily/metadata.json @@ -1,5 +1,5 @@ { - "title": "Assignment operators should return non-\"const\" references", + "title": "Assignment operators should return non-\"const\" reference to assigned object", "type": "CODE_SMELL", "code": { "impacts": { diff --git a/rules/S1236/cfamily/rule.adoc b/rules/S1236/cfamily/rule.adoc index 6e5f65e9578..99016c074fb 100644 --- a/rules/S1236/cfamily/rule.adoc +++ b/rules/S1236/cfamily/rule.adoc @@ -2,36 +2,179 @@ Copy assignment operators and move assignment operators can return anything, including ``++void++``. +However, if you decide to declare them yourself (don't forget the "Rule-of-Zero", S4963), +it is a recommended practice to return a non-const reference to the assigned object (left-operand). +It allows the developer to chain the assignment operations, increasing consistency with what other types do, and in some cases enabling writing concise code. -However, if you decide to declare them yourself (don't forget the "Rule-of-Zero", S4963), it is a recommended practice to return a non-const reference to the left-operand. It allows the developer to chain the assignment operations, increasing consistency with what other types do, and in some cases enabling writing concise code. +This rule will raise issue for the code construct, that deviate from above practice. +=== Using unconventional return type -=== Noncompliant code example +This rule will raise issue if the return type of the copy or move assigment operator, +is different from mutable reference to the class type. -[source,cpp] +==== Noncompliant code example + +[source,cpp,diff-id=1,diff-type=noncompliant] +---- +class Clazz { +public: + const Clazz& operator=(const Clazz& other) ; // Noncompliant, returns const reference + Clazz operator=(Clazz&& other) noexcept; // Noncompliant, returns by value +}; ---- -class A { + +==== Compliant solution + +[source,cpp,diff-id=1,diff-type=compliant] +---- +class Clazz { public: - ~A() = default; - A(A const &) = default; - A(A&&) = default; - const A& operator=(const A& other) ; // Noncompliant - A operator=(A&& other) noexcept; // Noncompliant + Clazz& operator=(const Clazz& other) ; // Compliant + Clazz& operator=(Clazz&& other) noexcept; // Compliant }; ---- +=== Returning object different that `*this` + +Assigmned operator should return reference to the assigned object. +Conventionally such return is expressed as `return *this`, and the rule will mark any of return stmt is deviating from this convetion. + +==== Noncompliant code example + +[source,cpp,diff-id=2,diff-type=noncompliant] +---- +class Clazz { +public: + Clazz& set(Clazz& other); + Clazz& operator=(Clazz const& other) { + return set(other); // Noncompliant, depends on return of `set` member function + } + + Clazz&& operator=(Clazz&& other) noexcept { + return other; // Noncompliant, also return type is non-compliant + } +}; +---- + +==== Compliant solution + +[source,cpp,diff-id=2,diff-type=compliant] +---- +class Clazz { +public: + Clazz& set(Clazz& other); + Clazz& operator=(Clazz const& other) { + set(other); + return *this; // Compliant + } + + Clazz& operator=(Clazz&& other) noexcept { + return *this; // Compliant + } +}; +---- + +In {cpp}23, if the assigmend operator is declared using explicit object argument, +the rule will mark any return statment that does not return object parameter directly. + +==== Noncompliant code example + +[source,cpp,diff-id=3,diff-type=noncompliant] +---- +class Clazz { +public: + Clazz& set(Clazz& other); + Clazz& operator=(this Clazz& self, Clazz const& other) { + return self.set(other); // Noncompliant, depends on return of `set` member function + } +}; +---- + +==== Compliant solution + +[source,cpp,diff-id=3,diff-type=compliant] +---- +class Clazz { +public: + Clazz& set(Clazz& other); + Clazz& operator=(this Clazz& self, Clazz const& other) { + self.set(other); + return self; // Compliant + } +}; +---- + +=== Declaring assigment operation as non-mutating + +The assigment operation is designed to change the value of target object, +to same one as the source. +Such operations are mutating and thus should not be declared with `const` qualifier. + +==== Noncompliant code example + +[source,cpp,diff-id=4,diff-type=noncompliant] +---- +class Clazz { +public: + Clazz& operator=(Clazz const& other) const { // Noncompliant, also least to noncompliant return statement + return const_cast(*this); + } + void operator=(Clazz&& other) const; // Noncompliant, with noncompliant return type +}; +---- + +==== Compliant solution + +[source,cpp,diff-id=4,diff-type=compliant] +---- +class Clazz { +public: + Clazz& operator=(Clazz const& other) const { // Compliant + return *this; + } + Clazz& operator=(Clazz&& other) const; // Compliant +}; +---- + +When declaring assigment operator with {cpp}23 explicit object argument, +an assigment operator can be declared as non-mutation by either passing object argument: + +* by const reference - this is equivalent to declaring method as const +* by value - in this case a temporary object will be created, and modified by assigment operator, + instead of lhs of assigment operator + +==== Noncompliant code example + +[source,cpp,diff-id=5,diff-type=noncompliant] +---- +class Clazz { + int val; +public: + Clazz& operator=(this Clazz const& self, Clazz const& other) const { // Noncompliant, also leads to non-compliant return + return const_cast(self); + } + void operator=(this Clazz self, Clazz&& other) const { // Noncompliant, with noncompliant return type + self.val = other.val; // Modifies temporary object + } +}; +---- -=== Compliant solution +==== Compliant solution -[source,cpp] +[source,cpp,diff-id=5,diff-type=compliant] ---- -class A { +class Clazz { + int val; public: - ~A() = default; - A(A const &) = default; - A(A&&) = default; - A& operator=(const A& other); - A& operator=(A&& other) noexcept; + Clazz& operator=(this Clazz& self, Clazz const& other) const { // Compliant + self.val = other.val; + return self; + } + Clazz& operator=(this Clazz& self, Clazz&& other) const { // Compliant + self.val = other.val; // Modifies referenced object + return self; + } }; ---- From d6535645b51b26bbe09c4fb918f084de10a08a22 Mon Sep 17 00:00:00 2001 From: tomasz-kaminski-sonarsource <79814193+tomasz-kaminski-sonarsource@users.noreply.github.com> Date: Wed, 8 May 2024 16:29:49 +0200 Subject: [PATCH 2/7] Grammarly pass --- rules/S1236/cfamily/rule.adoc | 41 ++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/rules/S1236/cfamily/rule.adoc b/rules/S1236/cfamily/rule.adoc index 99016c074fb..91d3f1e5393 100644 --- a/rules/S1236/cfamily/rule.adoc +++ b/rules/S1236/cfamily/rule.adoc @@ -4,13 +4,13 @@ Copy assignment operators and move assignment operators can return anything, inc However, if you decide to declare them yourself (don't forget the "Rule-of-Zero", S4963), it is a recommended practice to return a non-const reference to the assigned object (left-operand). -It allows the developer to chain the assignment operations, increasing consistency with what other types do, and in some cases enabling writing concise code. +It allows the developer to chain the assignment operations, increasing consistency with what other types do and, in some cases, enabling the writing of concise code. -This rule will raise issue for the code construct, that deviate from above practice. +This rule will raise for assignment operators that deviate from the above expectation. === Using unconventional return type -This rule will raise issue if the return type of the copy or move assigment operator, +This rule will raise an issue if the return type of the copy or move assignment operator, is different from mutable reference to the class type. ==== Noncompliant code example @@ -37,8 +37,8 @@ public: === Returning object different that `*this` -Assigmned operator should return reference to the assigned object. -Conventionally such return is expressed as `return *this`, and the rule will mark any of return stmt is deviating from this convetion. +An assignment operator should return a reference to the assigned object. +Conventionally, such return is expressed as `return *this`, and the rule will mark any return statement as deviating from this convention. ==== Noncompliant code example @@ -75,8 +75,8 @@ public: }; ---- -In {cpp}23, if the assigmend operator is declared using explicit object argument, -the rule will mark any return statment that does not return object parameter directly. +In {cpp}23, if the assignment operator is declared using an explicit object argument, +the rule will mark any return statement that does not return the object parameter directly. ==== Noncompliant code example @@ -86,7 +86,7 @@ class Clazz { public: Clazz& set(Clazz& other); Clazz& operator=(this Clazz& self, Clazz const& other) { - return self.set(other); // Noncompliant, depends on return of `set` member function + return self.set(other); // Noncompliant, depends on the return of `set` member function } }; ---- @@ -105,11 +105,11 @@ public: }; ---- -=== Declaring assigment operation as non-mutating +=== Declaring assignment operation as non-mutating -The assigment operation is designed to change the value of target object, -to same one as the source. -Such operations are mutating and thus should not be declared with `const` qualifier. +The assignment operation is designed to change the value of the target object, +to the same one as the source. +Such operations are mutating and thus should not be declared with a `const` qualifier. ==== Noncompliant code example @@ -137,12 +137,13 @@ public: }; ---- -When declaring assigment operator with {cpp}23 explicit object argument, -an assigment operator can be declared as non-mutation by either passing object argument: +When declaring assignment operator with {cpp}23 explicit object argument, +an assignment operator can be declared as non-mutation by either passing object argument: -* by const reference - this is equivalent to declaring method as const -* by value - in this case a temporary object will be created, and modified by assigment operator, - instead of lhs of assigment operator +* by const reference - this is equivalent to declaring the implicit object parameter method as `const`, + as described above; +* by value - in this case a temporary object will be created, and modified by the assignment operator, + instead of the left-hand side of the assignment operator ==== Noncompliant code example @@ -154,7 +155,7 @@ public: Clazz& operator=(this Clazz const& self, Clazz const& other) const { // Noncompliant, also leads to non-compliant return return const_cast(self); } - void operator=(this Clazz self, Clazz&& other) const { // Noncompliant, with noncompliant return type + void operator=(this Clazz self, Clazz&& other) { // Noncompliant, with noncompliant return type self.val = other.val; // Modifies temporary object } }; @@ -167,11 +168,11 @@ public: class Clazz { int val; public: - Clazz& operator=(this Clazz& self, Clazz const& other) const { // Compliant + Clazz& operator=(this Clazz& self, Clazz const& other) { // Compliant self.val = other.val; return self; } - Clazz& operator=(this Clazz& self, Clazz&& other) const { // Compliant + Clazz& operator=(this Clazz& self, Clazz&& other) { // Compliant self.val = other.val; // Modifies referenced object return self; } From b85e721658dfd63c2e1f7f47e7279142e4f795a1 Mon Sep 17 00:00:00 2001 From: tomasz-kaminski-sonarsource <79814193+tomasz-kaminski-sonarsource@users.noreply.github.com> Date: Wed, 8 May 2024 16:41:05 +0200 Subject: [PATCH 3/7] Corrected asciidoc --- rules/S1236/cfamily/rule.adoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rules/S1236/cfamily/rule.adoc b/rules/S1236/cfamily/rule.adoc index 91d3f1e5393..0d74a633a9e 100644 --- a/rules/S1236/cfamily/rule.adoc +++ b/rules/S1236/cfamily/rule.adoc @@ -35,10 +35,10 @@ public: }; ---- -=== Returning object different that `*this` +=== Returning object different that ``++*this++`` An assignment operator should return a reference to the assigned object. -Conventionally, such return is expressed as `return *this`, and the rule will mark any return statement as deviating from this convention. +Conventionally, such return is expressed as ``++return *this++``, and the rule will mark any return statement as deviating from this convention. ==== Noncompliant code example From 1a0d4aa4575a92c9c02c97eb59e1f7cbfafdecfb Mon Sep 17 00:00:00 2001 From: Tomasz Kaminski Date: Thu, 9 May 2024 08:35:09 +0200 Subject: [PATCH 4/7] Describe exception for void --- rules/S1236/cfamily/rule.adoc | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/rules/S1236/cfamily/rule.adoc b/rules/S1236/cfamily/rule.adoc index 0d74a633a9e..51311c54c6a 100644 --- a/rules/S1236/cfamily/rule.adoc +++ b/rules/S1236/cfamily/rule.adoc @@ -19,8 +19,8 @@ is different from mutable reference to the class type. ---- class Clazz { public: - const Clazz& operator=(const Clazz& other) ; // Noncompliant, returns const reference - Clazz operator=(Clazz&& other) noexcept; // Noncompliant, returns by value + const Clazz& operator=(const Clazz& other); // Noncompliant, returns const reference + Clazz operator=(Clazz&& other) noexcept; // Noncompliant, returns by value }; ---- @@ -30,7 +30,7 @@ public: ---- class Clazz { public: - Clazz& operator=(const Clazz& other) ; // Compliant + Clazz& operator=(const Clazz& other); // Compliant Clazz& operator=(Clazz&& other) noexcept; // Compliant }; ---- @@ -120,7 +120,7 @@ public: Clazz& operator=(Clazz const& other) const { // Noncompliant, also least to noncompliant return statement return const_cast(*this); } - void operator=(Clazz&& other) const; // Noncompliant, with noncompliant return type + Class& operator=(Clazz&& other) const; // Noncompliant }; ---- @@ -155,7 +155,7 @@ public: Clazz& operator=(this Clazz const& self, Clazz const& other) const { // Noncompliant, also leads to non-compliant return return const_cast(self); } - void operator=(this Clazz self, Clazz&& other) { // Noncompliant, with noncompliant return type + void operator=(this Clazz self, Clazz&& other) { // Noncompliant self.val = other.val; // Modifies temporary object } }; @@ -179,6 +179,25 @@ public: }; ---- +=== Exceptions + +This rule will not raise issue, for the return type of the assigment operator, that is declared as `void`. +Such syntax is commonly used in situation whe chaning of the assigment operator is not desired. +The issue will still be raised if such assigment operator is declared as non-mutating. + +[source,cpp] +---- +class Clazz { + int val; +public: + void operator=(Clazz const& other) { // Compliant + self.val = other.val; + return self; + } + void operator=(Clazz&& other) const; // Noncomplaint, declared as const +}; +---- + == Resources From ad4a7df82cbc6cd172baad59607e40cb18b89cd0 Mon Sep 17 00:00:00 2001 From: tomasz-kaminski-sonarsource <79814193+tomasz-kaminski-sonarsource@users.noreply.github.com> Date: Thu, 9 May 2024 08:40:09 +0200 Subject: [PATCH 5/7] Update rule.adoc --- rules/S1236/cfamily/rule.adoc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rules/S1236/cfamily/rule.adoc b/rules/S1236/cfamily/rule.adoc index 51311c54c6a..b2873dde0f9 100644 --- a/rules/S1236/cfamily/rule.adoc +++ b/rules/S1236/cfamily/rule.adoc @@ -181,9 +181,9 @@ public: === Exceptions -This rule will not raise issue, for the return type of the assigment operator, that is declared as `void`. -Such syntax is commonly used in situation whe chaning of the assigment operator is not desired. -The issue will still be raised if such assigment operator is declared as non-mutating. +This rule will not raise the issue for the return type of the assignment operator which is declared as `void.` +Such syntax is commonly used in situations where changing the assignment operator is not desired. +The issue will still be raised if such an assignment operator is declared as non-mutating. [source,cpp] ---- From 80b47489ca39f0bd5e27a19fec4827ba5de64af2 Mon Sep 17 00:00:00 2001 From: tomasz-kaminski-sonarsource <79814193+tomasz-kaminski-sonarsource@users.noreply.github.com> Date: Fri, 10 May 2024 15:47:35 +0200 Subject: [PATCH 6/7] Apply suggestions from code review Co-authored-by: Fred Tingaud <95592999+frederic-tingaud-sonarsource@users.noreply.github.com> --- rules/S1236/cfamily/metadata.json | 2 +- rules/S1236/cfamily/rule.adoc | 24 ++++++++++++------------ 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/rules/S1236/cfamily/metadata.json b/rules/S1236/cfamily/metadata.json index 8ae76d788c8..c091fd55d2d 100644 --- a/rules/S1236/cfamily/metadata.json +++ b/rules/S1236/cfamily/metadata.json @@ -1,5 +1,5 @@ { - "title": "Assignment operators should return non-\"const\" reference to assigned object", + "title": "Assignment operators should return non-\"const\" reference to the assigned object", "type": "CODE_SMELL", "code": { "impacts": { diff --git a/rules/S1236/cfamily/rule.adoc b/rules/S1236/cfamily/rule.adoc index b2873dde0f9..d7a6e36a2ab 100644 --- a/rules/S1236/cfamily/rule.adoc +++ b/rules/S1236/cfamily/rule.adoc @@ -8,7 +8,7 @@ It allows the developer to chain the assignment operations, increasing consisten This rule will raise for assignment operators that deviate from the above expectation. -=== Using unconventional return type +=== Using an unconventional return type This rule will raise an issue if the return type of the copy or move assignment operator, is different from mutable reference to the class type. @@ -35,7 +35,7 @@ public: }; ---- -=== Returning object different that ``++*this++`` +=== Returning an object different from ``++*this++`` An assignment operator should return a reference to the assigned object. Conventionally, such return is expressed as ``++return *this++``, and the rule will mark any return statement as deviating from this convention. @@ -48,7 +48,7 @@ class Clazz { public: Clazz& set(Clazz& other); Clazz& operator=(Clazz const& other) { - return set(other); // Noncompliant, depends on return of `set` member function + return set(other); // Noncompliant: depends on return of `set` member function } Clazz&& operator=(Clazz&& other) noexcept { @@ -109,7 +109,7 @@ public: The assignment operation is designed to change the value of the target object, to the same one as the source. -Such operations are mutating and thus should not be declared with a `const` qualifier. +Such operation is mutating and thus should not be declared with a `const` qualifier. ==== Noncompliant code example @@ -117,7 +117,7 @@ Such operations are mutating and thus should not be declared with a `const` qual ---- class Clazz { public: - Clazz& operator=(Clazz const& other) const { // Noncompliant, also least to noncompliant return statement + Clazz& operator=(Clazz const& other) const { // Noncompliant, also leads to noncompliant return statement return const_cast(*this); } Class& operator=(Clazz&& other) const; // Noncompliant @@ -130,15 +130,15 @@ public: ---- class Clazz { public: - Clazz& operator=(Clazz const& other) const { // Compliant + Clazz& operator=(Clazz const& other) { // Compliant return *this; } - Clazz& operator=(Clazz&& other) const; // Compliant + Clazz& operator=(Clazz&& other); // Compliant }; ---- -When declaring assignment operator with {cpp}23 explicit object argument, -an assignment operator can be declared as non-mutation by either passing object argument: +When declaring an assignment operator with {cpp}23 explicit object argument, +the object argument should not be passed: * by const reference - this is equivalent to declaring the implicit object parameter method as `const`, as described above; @@ -181,8 +181,8 @@ public: === Exceptions -This rule will not raise the issue for the return type of the assignment operator which is declared as `void.` -Such syntax is commonly used in situations where changing the assignment operator is not desired. +This rule will not raise an issue when the assignment operator's return type is declared `void.` +That syntax is commonly used when assignment operator chaining is not desired. The issue will still be raised if such an assignment operator is declared as non-mutating. [source,cpp] @@ -194,7 +194,7 @@ public: self.val = other.val; return self; } - void operator=(Clazz&& other) const; // Noncomplaint, declared as const + void operator=(Clazz&& other) const; // Noncompliant: declared as const }; ---- From 5633f2f754bd1cb5e32e82ca8fb0df5f0f470445 Mon Sep 17 00:00:00 2001 From: tomasz-kaminski-sonarsource <79814193+tomasz-kaminski-sonarsource@users.noreply.github.com> Date: Fri, 10 May 2024 16:00:12 +0200 Subject: [PATCH 7/7] Apply suggestions from code review Co-authored-by: Fred Tingaud <95592999+frederic-tingaud-sonarsource@users.noreply.github.com> --- rules/S1236/cfamily/rule.adoc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rules/S1236/cfamily/rule.adoc b/rules/S1236/cfamily/rule.adoc index d7a6e36a2ab..fad6e7a9073 100644 --- a/rules/S1236/cfamily/rule.adoc +++ b/rules/S1236/cfamily/rule.adoc @@ -86,7 +86,7 @@ class Clazz { public: Clazz& set(Clazz& other); Clazz& operator=(this Clazz& self, Clazz const& other) { - return self.set(other); // Noncompliant, depends on the return of `set` member function + return self.set(other); // Noncompliant: depends on the return of `set` member function } }; ---- @@ -117,7 +117,7 @@ Such operation is mutating and thus should not be declared with a `const` qualif ---- class Clazz { public: - Clazz& operator=(Clazz const& other) const { // Noncompliant, also leads to noncompliant return statement + Clazz& operator=(Clazz const& other) const { // Noncompliant: also leads to noncompliant return statement return const_cast(*this); } Class& operator=(Clazz&& other) const; // Noncompliant @@ -152,7 +152,7 @@ the object argument should not be passed: class Clazz { int val; public: - Clazz& operator=(this Clazz const& self, Clazz const& other) const { // Noncompliant, also leads to non-compliant return + Clazz& operator=(this Clazz const& self, Clazz const& other) const { // Noncompliant: also leads to non-compliant return return const_cast(self); } void operator=(this Clazz self, Clazz&& other) { // Noncompliant