Address feedback

Author: MichaelRFairhurst

cpp/misra/src/rules/RULE-13-3-1/MemberSpecifiersNotUsedAppropriately.ql

@@ -25,11 +25,11 @@ where
     f.isDeclaredVirtual() and
     exists(Specifier s |
       s = f.getASpecifier() and
-      s.getName() = ["final", "override"]
-    ) and
-    message =
-      "Member function '" + f.getName() +
-        "' uses redundant 'virtual' and 'final' specifiers together."
+      s.getName() = ["final", "override"] and
+      message =
+        "Member function '" + f.getName() + "' uses redundant 'virtual' and '" + s.getName() +
+          "' specifiers together."
+    )
     or
     // Case 2: Redundant 'virtual' specifier
     f.overrides(_) and

cpp/misra/src/rules/RULE-14-1-1/PrivateAndPublicDataMembersMixed.ql

@@ -27,6 +27,8 @@ where
     or
     // Public and private data members cannot be mixed.
     exists(Field other |
+      // Allow manual exclusion of 'other' field.
+      not isExcluded(other, Classes2Package::privateAndPublicDataMembersMixedQuery()) and
       other = c.getAMember() and
       not other.isStatic() and
       other != f and

cpp/misra/src/rules/RULE-15-0-2/InvalidSignatureForSpecialMemberFunction.ql

@@ -23,20 +23,21 @@ predicate checkSignature(
   MemberFunction f, Boolean noexcept, Boolean lvalueQualified, Boolean rvalueRef, string err
 ) {
   f.getNumberOfParameters() > 1 and
-  err = "too many parameters"
+  err = "has too many parameters"
   or
   noexcept = true and
   not f.isNoExcept() and
   err = "should be noexcept"
   or
+  // Note: There is no check when `lvalueQualified` is false, only when true. `lvalueQualified` will
+  // only be false for constructors, and constructors cannot be ref-qualified.
   lvalueQualified = true and
   not f.isLValueRefQualified() and
   err = "should be lvalue-qualified"
   or
-  lvalueQualified = false and
-  f.isLValueRefQualified() and
-  err = "should not be lvalue-qualified"
-  or
+  // Note: This case is also only here for completeness. `rvalueRef` is only true for move
+  // constructors and move assignment operators. However, these special member functions by
+  // definition must take an rvalue reference, so this case cannot hold.
   rvalueRef = true and
   not isRvalueRefX(f.getParameter(0).getType(), f.getDeclaringType()) and
   err = "should take rvalue reference to class or struct type"
@@ -53,6 +54,8 @@ predicate checkSignature(
   not f.getType() instanceof VoidType and
   err = "should return void or lvalue reference to class or struct type"
   or
+  // Note: this is here for completeness. It is a static error to declare copy or move constructors
+  // as `explicit`, so this cannot hold.
   not f instanceof Constructor and
   f.isExplicit() and
   err = "should not be explicit"

cpp/misra/test/rules/RULE-13-3-1/MemberSpecifiersNotUsedAppropriately.expected

@@ -2,12 +2,12 @@
 | test.cpp:15:16:15:17 | f2 | Member function 'f2' uses redundant 'virtual' and 'final' specifiers together. |
 | test.cpp:33:16:33:25 | m1_virtual | Member function 'm1_virtual' overrides a base function but uses 'virtual' specifier. |
 | test.cpp:38:16:38:25 | m1_virtual | Member function 'm1_virtual' overrides a base function but uses 'virtual' specifier. |
-| test.cpp:38:16:38:25 | m1_virtual | Member function 'm1_virtual' uses redundant 'virtual' and 'final' specifiers together. |
+| test.cpp:38:16:38:25 | m1_virtual | Member function 'm1_virtual' uses redundant 'virtual' and 'override' specifiers together. |
 | test.cpp:43:16:43:25 | m1_virtual | Member function 'm1_virtual' overrides a base function but uses 'virtual' specifier. |
 | test.cpp:43:16:43:25 | m1_virtual | Member function 'm1_virtual' uses redundant 'virtual' and 'final' specifiers together. |
 | test.cpp:48:8:48:17 | m1_virtual | Member function 'm1_virtual' uses redundant 'override' and 'final' specifiers together. |
 | test.cpp:53:8:53:17 | m1_virtual | Member function 'm1_virtual' overrides a base function but lacks 'override' or 'final' specifier. |
 | test.cpp:69:16:69:28 | m1_nonvirtual | Member function 'm1_nonvirtual' uses redundant 'virtual' and 'final' specifiers together. |
 | test.cpp:90:3:90:6 | ~C12 | Member function '~C12' overrides a base function but lacks 'override' or 'final' specifier. |
 | test.cpp:95:11:95:14 | ~C13 | Member function '~C13' overrides a base function but uses 'virtual' specifier. |
-| test.cpp:95:11:95:14 | ~C13 | Member function '~C13' uses redundant 'virtual' and 'final' specifiers together. |
+| test.cpp:95:11:95:14 | ~C13 | Member function '~C13' uses redundant 'virtual' and 'override' specifiers together. |

cpp/misra/test/rules/RULE-15-0-2/InvalidSignatureForSpecialMemberFunction.expected

@@ -7,5 +7,17 @@
 | test.cpp:92:3:92:22 | NonCompliantVolatile | Copy constructor on type $@ should take const reference to class or struct type. | test.cpp:89:7:89:26 | NonCompliantVolatile | NonCompliantVolatile |
 | test.cpp:100:3:100:11 | operator= | Copy assignment operator on type $@ should not be virtual. | test.cpp:96:7:96:25 | NonCompliantVirtual | NonCompliantVirtual |
 | test.cpp:107:3:107:11 | operator= | Copy assignment operator on type $@ should take const reference to class or struct type. | test.cpp:103:7:103:29 | NonCompliantCopyByValue | NonCompliantCopyByValue |
-| test.cpp:113:3:113:30 | NonCompliantDefaultArguments | Move constructor on type $@ too many parameters. | test.cpp:110:7:110:34 | NonCompliantDefaultArguments | NonCompliantDefaultArguments |
+| test.cpp:113:3:113:30 | NonCompliantDefaultArguments | Move constructor on type $@ has too many parameters. | test.cpp:110:7:110:34 | NonCompliantDefaultArguments | NonCompliantDefaultArguments |
 | test.cpp:158:3:158:20 | NonCompliantStruct | Copy constructor on type $@ should take const reference to class or struct type. | test.cpp:155:8:155:25 | NonCompliantStruct | NonCompliantStruct |
+| test.cpp:185:27:185:35 | operator= | Copy assignment operator on type $@ should return void or lvalue reference to class or struct type. | test.cpp:183:7:183:29 | AssignmentReturnByValue | AssignmentReturnByValue |
+| test.cpp:187:27:187:35 | operator= | Move assignment operator on type $@ should return void or lvalue reference to class or struct type. | test.cpp:183:7:183:29 | AssignmentReturnByValue | AssignmentReturnByValue |
+| test.cpp:193:31:193:39 | operator= | Copy assignment operator on type $@ should return void or lvalue reference to class or struct type. | test.cpp:191:7:191:31 | AssignmentReturnRvalueRef | AssignmentReturnRvalueRef |
+| test.cpp:195:31:195:39 | operator= | Move assignment operator on type $@ should return void or lvalue reference to class or struct type. | test.cpp:191:7:191:31 | AssignmentReturnRvalueRef | AssignmentReturnRvalueRef |
+| test.cpp:202:15:202:23 | operator= | Copy assignment operator on type $@ should return void or lvalue reference to class or struct type. | test.cpp:200:7:200:32 | WrongClassAssignmentReturn | WrongClassAssignmentReturn |
+| test.cpp:204:15:204:23 | operator= | Move assignment operator on type $@ should return void or lvalue reference to class or struct type. | test.cpp:200:7:200:32 | WrongClassAssignmentReturn | WrongClassAssignmentReturn |
+| test.cpp:210:7:210:15 | operator= | Copy assignment operator on type $@ should return void or lvalue reference to class or struct type. | test.cpp:208:7:208:22 | ScalarReturnType | ScalarReturnType |
+| test.cpp:212:7:212:15 | operator= | Move assignment operator on type $@ should return void or lvalue reference to class or struct type. | test.cpp:208:7:208:22 | ScalarReturnType | ScalarReturnType |
+| test.cpp:219:3:219:11 | operator= | Copy assignment operator on type $@ should return void or lvalue reference to class or struct type. | test.cpp:216:7:216:23 | PointerReturnType | PointerReturnType |
+| test.cpp:220:22:220:30 | operator= | Move assignment operator on type $@ should return void or lvalue reference to class or struct type. | test.cpp:216:7:216:23 | PointerReturnType | PointerReturnType |
+| test.cpp:226:8:226:16 | operator= | Copy assignment operator on type $@ should be lvalue-qualified. | test.cpp:224:7:224:34 | RvalueRefQualifiedAssignment | RvalueRefQualifiedAssignment |
+| test.cpp:228:8:228:16 | operator= | Move assignment operator on type $@ should be lvalue-qualified. | test.cpp:224:7:224:34 | RvalueRefQualifiedAssignment | RvalueRefQualifiedAssignment |

cpp/misra/test/rules/RULE-15-0-2/test.cpp

@@ -8,10 +8,10 @@
 class CompliantClass {
 public:
   CompliantClass() = default;
-  CompliantClass(CompliantClass const &);                 // COMPLIANT
-  CompliantClass(CompliantClass &&) noexcept;             // COMPLIANT
-  CompliantClass &operator=(CompliantClass const &) &;    // COMPLIANT
-  CompliantClass &operator=(CompliantClass &&) &noexcept; // COMPLIANT
+  CompliantClass(CompliantClass const &);                  // COMPLIANT
+  CompliantClass(CompliantClass &&) noexcept;              // COMPLIANT
+  CompliantClass &operator=(CompliantClass const &) &;     // COMPLIANT
+  CompliantClass &operator=(CompliantClass &&) & noexcept; // COMPLIANT
 };
 
 class CompliantWithAlternatives {
@@ -22,9 +22,9 @@ class CompliantWithAlternatives {
   explicit constexpr CompliantWithAlternatives(
       CompliantWithAlternatives &&) noexcept; // COMPLIANT
   constexpr CompliantWithAlternatives &
-  operator=(const CompliantWithAlternatives &) &noexcept; // COMPLIANT
+  operator=(const CompliantWithAlternatives &) & noexcept; // COMPLIANT
   constexpr CompliantWithAlternatives &
-  operator=(CompliantWithAlternatives &&) &noexcept; // COMPLIANT
+  operator=(CompliantWithAlternatives &&) & noexcept; // COMPLIANT
 };
 
 class CompliantVoidReturn {
@@ -35,7 +35,7 @@ class CompliantVoidReturn {
   void
   operator=(CompliantVoidReturn const &) &; // COMPLIANT - void return allowed
   void operator=(
-      CompliantVoidReturn &&) &noexcept; // COMPLIANT - void return allowed
+      CompliantVoidReturn &&) & noexcept; // COMPLIANT - void return allowed
 };
 
 // Non-compliant examples
@@ -146,15 +146,93 @@ class DeletedOperations {
 struct CompliantStruct {
 public:
   CompliantStruct() = default;
-  CompliantStruct(CompliantStruct const &);                 // COMPLIANT
-  CompliantStruct(CompliantStruct &&) noexcept;             // COMPLIANT
-  CompliantStruct &operator=(CompliantStruct const &) &;    // COMPLIANT
-  CompliantStruct &operator=(CompliantStruct &&) &noexcept; // COMPLIANT
+  CompliantStruct(CompliantStruct const &);                  // COMPLIANT
+  CompliantStruct(CompliantStruct &&) noexcept;              // COMPLIANT
+  CompliantStruct &operator=(CompliantStruct const &) &;     // COMPLIANT
+  CompliantStruct &operator=(CompliantStruct &&) & noexcept; // COMPLIANT
 };
 
 struct NonCompliantStruct {
 public:
   NonCompliantStruct() = default;
   NonCompliantStruct(
       NonCompliantStruct &); // NON_COMPLIANT - non-const reference
-};
+};
+
+// Static error case. This would be debatably non-compliant if it were not a
+// static error, because by pure specificationrules, it is not a copy or move
+// constructor. OTOH, clang's error states that it's a malformed copy
+// constructor. Overall, there's a case to be made for compliant and
+// non-compliant, but either way we cannot test it here.
+// struct NonLvalueRefConstructor {
+// public:
+//  NonLvalueRefConstructor(NonLvalueRefConstructor n) noexcept; // COMPLIANT
+//}
+
+struct NonRvalueRefConstructorAndAssignment {
+public:
+  // This is a static error just like `NonLvalueRefConstructor`, for similar
+  // reasons.
+  // NonRvalueRefConstructorAndAssignment(NonRvalueRefConstructorAndAssignment
+  // n) noexcept;
+
+  // The following is already tested in `NonCompliantCopyByValue`
+  // void operator=(NonRvalueRefConstructorAndAssignment n);
+};
+
+class AssignmentReturnByValue {
+public:
+  AssignmentReturnByValue operator=(
+      AssignmentReturnByValue const &) &; // NON_COMPLIANT - wrong return type
+  AssignmentReturnByValue operator=(AssignmentReturnByValue &&)
+      & noexcept; // NON_COMPLIANT - wrong return type
+};
+
+class AssignmentReturnRvalueRef {
+public:
+  AssignmentReturnRvalueRef &&operator=(
+      AssignmentReturnRvalueRef const &) &; // NON_COMPLIANT - wrong return type
+  AssignmentReturnRvalueRef &&operator=(AssignmentReturnRvalueRef &&)
+      & noexcept; // NON_COMPLIANT - wrong return type
+};
+
+class WrongClass;
+class WrongClassAssignmentReturn {
+public:
+  WrongClass &operator=(WrongClassAssignmentReturn const &)
+      &; // NON_COMPLIANT - wrong return type
+  WrongClass &operator=(WrongClassAssignmentReturn &&)
+      & noexcept; // NON_COMPLIANT - wrong return type
+};
+
+class ScalarReturnType {
+public:
+  int operator=(
+      ScalarReturnType const &) &; // NON_COMPLIANT - wrong return type
+  int operator=(
+      ScalarReturnType &&) & noexcept; // NON_COMPLIANT - wrong return type
+};
+
+class PointerReturnType {
+public:
+  PointerReturnType *
+  operator=(PointerReturnType const &) &; // NON_COMPLIANT - wrong return type
+  PointerReturnType *operator=(
+      PointerReturnType &&) & noexcept; // NON_COMPLIANT - wrong return type
+};
+
+class RvalueRefQualifiedAssignment {
+public:
+  void operator=(const RvalueRefQualifiedAssignment &)
+      &&; // NON_COMPLIANT - rvalue ref qualified
+  void operator=(RvalueRefQualifiedAssignment &&)
+      && noexcept; // NON_COMPLIANT - rvalue ref qualified
+};
+
+// It is a static error to declare assignment operators as `explicit`.
+// class ExplicitAssignmentOperators {
+// public:
+//   explicit void operator=(const ExplicitAssignmentOperators &) &; //
+//   NON_COMPLIANT explicit void operator=(ExplicitAssignmentOperators &&) &; //
+//   NON_COMPLIANT
+// };