diff --git a/rules/S1236/cfamily/metadata.json b/rules/S1236/cfamily/metadata.json index 522863a342d..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\" references", + "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 6e5f65e9578..fad6e7a9073 100644 --- a/rules/S1236/cfamily/rule.adoc +++ b/rules/S1236/cfamily/rule.adoc @@ -2,36 +2,199 @@ 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 the writing of 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 for assignment operators that deviate from the above expectation. +=== Using an unconventional return type -=== Noncompliant code example +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. -[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 +}; +---- + +==== Compliant solution + +[source,cpp,diff-id=1,diff-type=compliant] +---- +class Clazz { +public: + Clazz& operator=(const Clazz& other); // Compliant + Clazz& operator=(Clazz&& other) noexcept; // Compliant +}; +---- + +=== 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. + +==== 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 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 + +[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 the 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 assignment operation as non-mutating + +The assignment operation is designed to change the value of the target object, +to the same one as the source. +Such operation is mutating and thus should not be declared with a `const` qualifier. + +==== Noncompliant code example + +[source,cpp,diff-id=4,diff-type=noncompliant] +---- +class Clazz { +public: + Clazz& operator=(Clazz const& other) const { // Noncompliant: also leads to noncompliant return statement + return const_cast(*this); + } + Class& operator=(Clazz&& other) const; // Noncompliant +}; +---- + +==== Compliant solution + +[source,cpp,diff-id=4,diff-type=compliant] +---- +class Clazz { +public: + Clazz& operator=(Clazz const& other) { // Compliant + return *this; + } + Clazz& operator=(Clazz&& other); // Compliant +}; +---- + +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; +* 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 + +[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) { // Noncompliant + self.val = other.val; // Modifies temporary object + } +}; +---- + +==== Compliant solution + +[source,cpp,diff-id=5,diff-type=compliant] ---- -class A { +class Clazz { + int val; 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=(this Clazz& self, Clazz const& other) { // Compliant + self.val = other.val; + return self; + } + Clazz& operator=(this Clazz& self, Clazz&& other) { // Compliant + self.val = other.val; // Modifies referenced object + return self; + } }; ---- +=== Exceptions -=== Compliant solution +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] ---- -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; + void operator=(Clazz const& other) { // Compliant + self.val = other.val; + return self; + } + void operator=(Clazz&& other) const; // Noncompliant: declared as const }; ----