Skip to content

Commit dab3192

Browse files
tzolnaitzolnai
committedMay 23, 2019
[clang-tidy]: Add cert-oop54-cpp alias for bugprone-unhandled-self-assignment
Summary: Added WarnOnlyIfThisHasSuspiciousField option to allow to catch any copy assignment operator independently from the container class's fields. Added the cert alias using this option. Reviewers: aaron.ballman Reviewed By: aaron.ballman Subscribers: mgorny, Eugene.Zelenko, xazax.hun, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D62192 llvm-svn: 361550
1 parent 14f4ff6 commit dab3192

10 files changed

+131
-26
lines changed
 

‎clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp

+38-23
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,18 @@ namespace clang {
1616
namespace tidy {
1717
namespace bugprone {
1818

19+
UnhandledSelfAssignmentCheck::UnhandledSelfAssignmentCheck(
20+
StringRef Name, ClangTidyContext *Context)
21+
: ClangTidyCheck(Name, Context),
22+
WarnOnlyIfThisHasSuspiciousField(
23+
Options.get("WarnOnlyIfThisHasSuspiciousField", true)) {}
24+
25+
void UnhandledSelfAssignmentCheck::storeOptions(
26+
ClangTidyOptions::OptionMap &Opts) {
27+
Options.store(Opts, "WarnOnlyIfThisHasSuspiciousField",
28+
WarnOnlyIfThisHasSuspiciousField);
29+
}
30+
1931
void UnhandledSelfAssignmentCheck::registerMatchers(MatchFinder *Finder) {
2032
if (!getLangOpts().CPlusPlus)
2133
return;
@@ -61,29 +73,32 @@ void UnhandledSelfAssignmentCheck::registerMatchers(MatchFinder *Finder) {
6173
cxxMethodDecl(unless(hasDescendant(cxxMemberCallExpr(callee(cxxMethodDecl(
6274
hasName("operator="), ofClass(equalsBoundNode("class"))))))));
6375

64-
// Matcher for standard smart pointers.
65-
const auto SmartPointerType = qualType(hasUnqualifiedDesugaredType(
66-
recordType(hasDeclaration(classTemplateSpecializationDecl(
67-
hasAnyName("::std::shared_ptr", "::std::unique_ptr",
68-
"::std::weak_ptr", "::std::auto_ptr"),
69-
templateArgumentCountIs(1))))));
70-
71-
// We will warn only if the class has a pointer or a C array field which
72-
// probably causes a problem during self-assignment (e.g. first resetting the
73-
// pointer member, then trying to access the object pointed by the pointer, or
74-
// memcpy overlapping arrays).
75-
const auto ThisHasSuspiciousField = cxxMethodDecl(ofClass(cxxRecordDecl(
76-
has(fieldDecl(anyOf(hasType(pointerType()), hasType(SmartPointerType),
77-
hasType(arrayType())))))));
78-
79-
Finder->addMatcher(
80-
cxxMethodDecl(ofClass(cxxRecordDecl().bind("class")),
81-
isCopyAssignmentOperator(), IsUserDefined,
82-
HasReferenceParam, HasNoSelfCheck,
83-
unless(HasNonTemplateSelfCopy), unless(HasTemplateSelfCopy),
84-
HasNoNestedSelfAssign, ThisHasSuspiciousField)
85-
.bind("copyAssignmentOperator"),
86-
this);
76+
DeclarationMatcher AdditionalMatcher = cxxMethodDecl();
77+
if (WarnOnlyIfThisHasSuspiciousField) {
78+
// Matcher for standard smart pointers.
79+
const auto SmartPointerType = qualType(hasUnqualifiedDesugaredType(
80+
recordType(hasDeclaration(classTemplateSpecializationDecl(
81+
hasAnyName("::std::shared_ptr", "::std::unique_ptr",
82+
"::std::weak_ptr", "::std::auto_ptr"),
83+
templateArgumentCountIs(1))))));
84+
85+
// We will warn only if the class has a pointer or a C array field which
86+
// probably causes a problem during self-assignment (e.g. first resetting
87+
// the pointer member, then trying to access the object pointed by the
88+
// pointer, or memcpy overlapping arrays).
89+
AdditionalMatcher = cxxMethodDecl(ofClass(cxxRecordDecl(
90+
has(fieldDecl(anyOf(hasType(pointerType()), hasType(SmartPointerType),
91+
hasType(arrayType())))))));
92+
}
93+
94+
Finder->addMatcher(cxxMethodDecl(ofClass(cxxRecordDecl().bind("class")),
95+
isCopyAssignmentOperator(), IsUserDefined,
96+
HasReferenceParam, HasNoSelfCheck,
97+
unless(HasNonTemplateSelfCopy),
98+
unless(HasTemplateSelfCopy),
99+
HasNoNestedSelfAssign, AdditionalMatcher)
100+
.bind("copyAssignmentOperator"),
101+
this);
87102
}
88103

89104
void UnhandledSelfAssignmentCheck::check(

‎clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h

+6-2
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,14 @@ namespace bugprone {
2323
/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-unhandled-self-assignment.html
2424
class UnhandledSelfAssignmentCheck : public ClangTidyCheck {
2525
public:
26-
UnhandledSelfAssignmentCheck(StringRef Name, ClangTidyContext *Context)
27-
: ClangTidyCheck(Name, Context) {}
26+
UnhandledSelfAssignmentCheck(StringRef Name, ClangTidyContext *Context);
27+
28+
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
2829
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
2930
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
31+
32+
private:
33+
const bool WarnOnlyIfThisHasSuspiciousField;
3034
};
3135

3236
} // namespace bugprone

‎clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp

+4
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include "../ClangTidy.h"
1010
#include "../ClangTidyModule.h"
1111
#include "../ClangTidyModuleRegistry.h"
12+
#include "../bugprone/UnhandledSelfAssignmentCheck.h"
1213
#include "../google/UnnamedNamespaceInHeaderCheck.h"
1314
#include "../misc/NewDeleteOverloadsCheck.h"
1415
#include "../misc/NonCopyableObjects.h"
@@ -49,6 +50,8 @@ class CERTModule : public ClangTidyModule {
4950
// OOP
5051
CheckFactories.registerCheck<performance::MoveConstructorInitCheck>(
5152
"cert-oop11-cpp");
53+
CheckFactories.registerCheck<bugprone::UnhandledSelfAssignmentCheck>(
54+
"cert-oop54-cpp");
5255
// ERR
5356
CheckFactories.registerCheck<misc::ThrowByValueCatchByReferenceCheck>(
5457
"cert-err09-cpp");
@@ -85,6 +88,7 @@ class CERTModule : public ClangTidyModule {
8588
ClangTidyOptions Options;
8689
ClangTidyOptions::OptionMap &Opts = Options.CheckOptions;
8790
Opts["cert-dcl16-c.NewSuffixes"] = "L;LL;LU;LLU";
91+
Opts["cert-oop54-cpp.WarnOnlyIfThisHasSuspiciousField"] = "0";
8892
return Options;
8993
}
9094
};

‎clang-tools-extra/clang-tidy/cert/CMakeLists.txt

+1
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ add_clang_library(clangTidyCERTModule
2020
clangBasic
2121
clangLex
2222
clangTidy
23+
clangTidyBugproneModule
2324
clangTidyGoogleModule
2425
clangTidyMiscModule
2526
clangTidyPerformanceModule

‎clang-tools-extra/docs/ReleaseNotes.rst

+5
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,11 @@ Improvements to clang-tidy
134134
subclasses of ``NSObject`` and recommends calling a superclass initializer
135135
instead.
136136

137+
- New alias :doc:`cert-oop54-cpp
138+
<clang-tidy/checks/cert-oop54-cpp>` to
139+
:doc:`bugprone-unhandled-self-assignment
140+
<clang-tidy/checks/bugprone-unhandled-self-assignment>` was added.
141+
137142
- New alias :doc:`cppcoreguidelines-explicit-virtual-functions
138143
<clang-tidy/checks/cppcoreguidelines-explicit-virtual-functions>` to
139144
:doc:`modernize-use-override

‎clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst

+9-1
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,14 @@
33
bugprone-unhandled-self-assignment
44
==================================
55

6+
`cert-oop54-cpp` redirects here as an alias for this check. For the CERT alias,
7+
the `WarnOnlyIfThisHasSuspiciousField` option is set to `0`.
8+
69
Finds user-defined copy assignment operators which do not protect the code
710
against self-assignment either by checking self-assignment explicitly or
811
using the copy-and-swap or the copy-and-move method.
912

10-
This check now searches only those classes which have any pointer or C array field
13+
By default, this check searches only those classes which have any pointer or C array field
1114
to avoid false positives. In case of a pointer or a C array, it's likely that self-copy
1215
assignment breaks the object if the copy assignment operator was not written with care.
1316

@@ -114,3 +117,8 @@ temporary object into ``this`` (needs a move assignment operator):
114117
return *this;
115118
}
116119
};
120+
121+
.. option:: WarnOnlyIfThisHasSuspiciousField
122+
123+
When non-zero, the check will warn only if the container class of the copy assignment operator
124+
has any suspicious fields (pointer or C array). This option is set to `1` by default.

‎clang-tools-extra/docs/clang-tidy/checks/cert-oop54-cpp.rst

+10
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
.. title:: clang-tidy - cert-oop54-cpp
2+
.. meta::
3+
:http-equiv=refresh: 5;URL=bugprone-unhandled-self-assignment.html
4+
5+
cert-oop54-cpp
6+
==============
7+
8+
The cert-oop54-cpp check is an alias, please see
9+
`bugprone-unhandled-self-assignment <bugprone-unhandled-self-assignment.html>`_
10+
for more information.

‎clang-tools-extra/docs/clang-tidy/checks/list.rst

+1
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ Clang-Tidy Checks
9898
cert-msc50-cpp
9999
cert-msc51-cpp
100100
cert-oop11-cpp (redirects to performance-move-constructor-init) <cert-oop11-cpp>
101+
cert-oop54-cpp (redirects to bugprone-unhandled-self-assignment) <cert-oop54-cpp>
101102
cppcoreguidelines-avoid-c-arrays (redirects to modernize-avoid-c-arrays) <cppcoreguidelines-avoid-c-arrays>
102103
cppcoreguidelines-avoid-goto
103104
cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) <cppcoreguidelines-avoid-magic-numbers>

‎clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment-warn-only-if-this-has-suspicious-field.cpp

+41
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
// RUN: %check_clang_tidy %s bugprone-unhandled-self-assignment %t -- \
2+
// RUN: -config="{CheckOptions: \
3+
// RUN: [{key: bugprone-unhandled-self-assignment.WarnOnlyIfThisHasSuspiciousField, \
4+
// RUN: value: 0}]}"
5+
6+
// Classes with pointer field are still caught.
7+
class PtrField {
8+
public:
9+
PtrField &operator=(const PtrField &object) {
10+
// CHECK-MESSAGES: [[@LINE-1]]:13: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
11+
return *this;
12+
}
13+
14+
private:
15+
int *p;
16+
};
17+
18+
// With the option, check catches classes with trivial fields.
19+
class TrivialFields {
20+
public:
21+
TrivialFields &operator=(const TrivialFields &object) {
22+
// CHECK-MESSAGES: [[@LINE-1]]:18: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
23+
return *this;
24+
}
25+
26+
private:
27+
int m;
28+
float f;
29+
double d;
30+
bool b;
31+
};
32+
33+
// The check warns also when there is no field at all.
34+
// In this case, user-defined copy assignment operator is useless anyway.
35+
class ClassWithoutFields {
36+
public:
37+
ClassWithoutFields &operator=(const ClassWithoutFields &object) {
38+
// CHECK-MESSAGES: [[@LINE-1]]:23: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
39+
return *this;
40+
}
41+
};

‎clang-tools-extra/test/clang-tidy/cert-oop54-cpp.cpp

+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
// RUN: %check_clang_tidy %s cert-oop54-cpp %t
2+
3+
// Test whether bugprone-unhandled-self-assignment.WarnOnlyIfThisHasSuspiciousField option is set correctly.
4+
class TrivialFields {
5+
public:
6+
TrivialFields &operator=(const TrivialFields &object) {
7+
// CHECK-MESSAGES: [[@LINE-1]]:18: warning: operator=() does not handle self-assignment properly [cert-oop54-cpp]
8+
return *this;
9+
}
10+
11+
private:
12+
int m;
13+
float f;
14+
double d;
15+
bool b;
16+
};

0 commit comments

Comments
 (0)
Please sign in to comment.