Skip to content

Commit e4cea41

Browse files
MichaelRFairhurstMichaelRFairhurst
committed
Address feedback
1 parent 61a4dd9 commit e4cea41

2 files changed

Lines changed: 116 additions & 18 deletions

File tree

‎cpp/misra/src/rules/RULE-15-0-1/AnalyzableClass.qll‎

Lines changed: 41 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -48,19 +48,54 @@ private predicate isMemberCustomized(MemberFunction f) {
4848
private predicate isUserDeclared(MemberFunction f) { not f.isCompilerGenerated() }
4949

5050
/**
51-
* Holds if the implicit move constructor or move assignment operator of the class `c` will not be
52-
* declared.
51+
* Holds if the class `c` contains a user declared copy constructor, copy assignment operator,
52+
* or destructor.
5353
*
54-
* Specified in [class.copy.ctor]/8 and [class.copy.assign]/4.
54+
* Encapsulates common behavior related to implicit move operation suppression defined in
55+
* [class.copy.ctor]/8 and [class.copy.assign]/4.
5556
*/
56-
private predicate implicitMoveIsSuppressed(Class c) {
57+
predicate hasUserDeclaredCopyOrDestruct(Class c) {
5758
isUserDeclared(c.getAConstructor().(CopyConstructor))
5859
or
5960
isUserDeclared(c.getAMemberFunction().(CopyAssignmentOperator))
6061
or
6162
isUserDeclared(c.getDestructor())
6263
}
6364

65+
/**
66+
* Holds if the implicit move constructor of the class `c` will not be declared, as specified in
67+
* [class.copy.ctor]/8.
68+
*/
69+
private predicate implicitMoveConstructorSuppressed(Class c) {
70+
// Sanity check: if the move constructor exists, it must not have been suppressed.
71+
not exists(c.getAConstructor().(MoveConstructor)) and
72+
(
73+
// Per the spec, the move constructor is suppressed if:
74+
// There is a user declared copy constructor, copy assignment operator, or destructor,
75+
hasUserDeclaredCopyOrDestruct(c)
76+
or
77+
// or, the move assignment operator is user declared.
78+
isUserDeclared(c.getAMemberFunction().(MoveAssignmentOperator))
79+
)
80+
}
81+
82+
/**
83+
* Holds if the implicit move assignment operator of the class `c` will not be declared, as specified
84+
* in [class.copy.assign]/4.
85+
*/
86+
private predicate implicitMoveAssignmentSuppressed(Class c) {
87+
// Sanity check: if the move assignment operator exists, it must not have been suppressed.
88+
not exists(c.getAMemberFunction().(MoveAssignmentOperator)) and
89+
(
90+
// Per the spec, the move assignment operator is suppressed if:
91+
// There is a user declared copy constructor, copy assignment operator, or destructor,
92+
hasUserDeclaredCopyOrDestruct(c)
93+
or
94+
// or, the move constructor is user declared.
95+
isUserDeclared(c.getAMemberFunction().(MoveConstructor))
96+
)
97+
}
98+
6499
/**
65100
* Returns the move constructor of the class `c` if it exists, or the copy constructor if it does
66101
* not exist and the implicit definition was suppressed by the compiler.
@@ -83,9 +118,7 @@ private predicate implicitMoveIsSuppressed(Class c) {
83118
* assertion in the above example would fail.
84119
*/
85120
private Constructor getMoveConstructor(Class c) {
86-
if
87-
not exists(MoveConstructor mc | mc = c.getAConstructor() and isUserDeclared(mc)) and
88-
implicitMoveIsSuppressed(c)
121+
if implicitMoveConstructorSuppressed(c)
89122
then result = c.getAConstructor().(CopyConstructor)
90123
else result = c.getAConstructor().(MoveConstructor)
91124
}
@@ -112,9 +145,7 @@ private Constructor getMoveConstructor(Class c) {
112145
* assertion in the above example would fail.
113146
*/
114147
private Operator getMoveAssign(Class c) {
115-
if
116-
not exists(MoveAssignmentOperator mc | mc = c.getAMemberFunction() and isUserDeclared(mc)) and
117-
implicitMoveIsSuppressed(c)
148+
if implicitMoveAssignmentSuppressed(c)
118149
then result = c.getAMemberFunction().(CopyAssignmentOperator)
119150
else result = c.getAMemberFunction().(MoveAssignmentOperator)
120151
}

‎cpp/misra/src/rules/RULE-15-0-1/ImproperlyProvidedSpecialMemberFunctions.ql‎

Lines changed: 75 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -80,18 +80,85 @@ predicate violatesCustomizedMoveOrCopyRequirements(AnalyzableClass c, string rea
8080
}
8181

8282
private predicate undeclaredMoveException(AnalyzableClass c) {
83-
// A copy-enabled class may have an undeclared move constructor.
8483
isCopyEnabled(c) and
85-
not c.copyAssignable() and
86-
not c.declaresMoveConstructor()
87-
or
88-
// A copy-assignable class may leave both move operations undeclared.
89-
c.copyAssignable() and
90-
not c.declaresMoveConstructor() and
91-
not c.declaresMoveAssignmentOperator()
84+
(
85+
if c.copyAssignable()
86+
then
87+
// A copy-assignable class may have an undeclared move constructor.
88+
not c.declaresMoveConstructor() and
89+
not c.declaresMoveAssignmentOperator()
90+
else
91+
// A copy-enabled class may have an undeclared move constructor.
92+
not c.declaresMoveConstructor()
93+
)
9294
}
9395

9496
predicate violatesCustomizedDestructorRequirements(AnalyzableClass c, string reason) {
97+
// This is is simplified from the exact MISRA spec in order to more closely enforce the general
98+
// idea that, if you customize the destructor, you generally need to customize the other special
99+
// member functions (e.g., ISO C++ Core Guidelines C.21 and C.22).
100+
//
101+
// The MISRA specification is subtle and arguably ambiguous. In our interpretation of the rules,
102+
// bullet 2 sentence 2 is deemed to only apply to move-only classes, otherwise it would conflict
103+
// with bullet 3 sentence 2. We do not strictly need to enforce any additional constraints on
104+
// bullet 3 sentence 2; all compliant copy-assignable classes must be copy-enabled.
105+
//
106+
// Here is a logical derivation that shows our logic is equivalent to the MISRA spec for classes
107+
// that fall into one of the compliant categories. For classes outside of those categories, we
108+
// essentially enforce C.21 and C.22 from the ISO C++ Core Guidelines.
109+
//
110+
// ```
111+
// -- For all rules, assume hasCustomizedDestructor(class).
112+
// isMoveOnly(class) -> hasCustomMoveConstructor(class)
113+
// isMoveOnly(class) &&
114+
// isMoveAssignable(class) -> hasCustomMoveConstructor(class) && hasCustomMoveAssignment(class)
115+
// isCopyEnabled(class) -> hasCustomCopyConstructor(class) && (hasCustomMoveConstructor(class)
116+
// || ~isMoveConstructorDeclared(class))
117+
// isCopyAssignable(class) -> hasCustomCopyAssignment(class)
118+
// && ( hasCustomMoveConstructor(class) && hasCustomMoveAssignment(class)
119+
// || ~isMoveConstructorDeclared(class) && ~isMoveAssignmentDeclared(class))
120+
//
121+
// -- Invert
122+
// hasCustomMoveConstructor(c) -> isMoveOnly(c)
123+
// || isMoveAssignable(c)
124+
// || (isCopyEnabled(c) && ~isMoveConstructorDeclared(c))
125+
// || (isCopyAssignable(c) && ~isMoveConstructorDeclared(c)
126+
// && ~isMoveAssignmentDeclared(c))
127+
//
128+
// hasCustomMoveAssignmentOperator(c) -> (isMoveOnly(class) && isMoveAssignable(c))
129+
// || isCopyAssignable(c) && ~isMoveConstructorDeclared(c)
130+
// && ~isMoveAssignmentDeclared(c))
131+
//
132+
// hasCustomCopyConstructor(c) -> isCopyEnabled(c)
133+
// || isCopyAssignable(c)
134+
//
135+
// hasCustomCopyAssignmentOperator(c) -> isCopyAssignable(c);
136+
//
137+
// -- Intermediate statements derived from table
138+
// isCopyAssignable(c) -> isMoveAssignable(c)
139+
// isMoveAssignable(c) -> isMoveConstructible(c)
140+
// isCopyEnabled(c) || isCopyAssignable(c) -> isCopyConstructible(c)
141+
// isMoveOnly(c) || isMoveAssignable(c) || isCopyEnabled(c) || isCopyAssignable(c) -> isMoveConstructible(c)
142+
//
143+
// -- Simplify
144+
// hasCustomMoveConstructor(c) -> isMoveConstructible(c)
145+
// || (isCopyEnabled(c) && ~isMoveConstructorDeclared(c))
146+
// || (isCopyAssignable(c) && ~isMoveConstructorDeclared(c) && ~isMoveAssignmentDeclared(c))
147+
// hasCustomMoveAssignmentOperator(c) -> isMoveAssignable(c)
148+
// || isCopyAssignable(c) && (~isMoveConstructorDeclared(c) && ~isMoveAssignmentDeclared(c))
149+
// hasCustomCopyConstructor(c) -> isCopyConstructible(c)
150+
// hasCustomCopyAssignmentOperator(c) -> isCopyAssignable(c)
151+
//
152+
// -- Extract exceptional cases
153+
// Exception1(class) :- isCopyEnabled(c) && ~isMoveConstructorDeclared(c)
154+
// Exception2(class) :- isCopyAssignable(c) && ~isMoveConstructorDeclared(c) && ~isMoveAssignmentDeclared(c)
155+
//
156+
// -- Simplify
157+
// hasCustomMoveConstructor(class) -> isMoveConstructible(c) || Exception1(c) || Exception2(c)
158+
// hasCustomMoveAssignmentOperator(c) -> isMoveAssignable(c) || Exception2(c)
159+
// hasCustomCopyConstructor(c) -> isCopyConstructible(c)
160+
// hasCustomCopyAssignmentOperator(c) -> isCopyAssignable(c);
161+
// ```
95162
c.isCustomized(TDestructor()) and
96163
(
97164
c.moveConstructible() and

0 commit comments

Comments
 (0)