Skip to content

Commit f2e5000

Browse files
loskutovPiotrZSL
loskutov
authored and
PiotrZSL
committed
[clang-tidy] Fix DanglingHandleCheck to work in C++17 and later mode
Due to guaranteed copy elision, not only do some nodes get removed from the AST, but also some existing nodes change the source locations they correspond to. Hence, the check works slightly differently in pre-C++17 and C++17-and-later modes in terms of what gets highlighted. Reviewed By: PiotrZSL Differential Revision: https://reviews.llvm.org/D158371
1 parent 01c1156 commit f2e5000

File tree

2 files changed

+48
-49
lines changed

2 files changed

+48
-49
lines changed

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

+32-42
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,9 @@ ast_matchers::internal::Matcher<Stmt> handleFromTemporaryValue(
3636
// If a ternary operator returns a temporary value, then both branches hold a
3737
// temporary value. If one of them is not a temporary then it must be copied
3838
// into one to satisfy the type of the operator.
39-
const auto TemporaryTernary =
40-
conditionalOperator(hasTrueExpression(cxxBindTemporaryExpr()),
41-
hasFalseExpression(cxxBindTemporaryExpr()));
39+
const auto TemporaryTernary = conditionalOperator(
40+
hasTrueExpression(ignoringParenImpCasts(cxxBindTemporaryExpr())),
41+
hasFalseExpression(ignoringParenImpCasts(cxxBindTemporaryExpr())));
4242

4343
return handleFrom(IsAHandle, anyOf(cxxBindTemporaryExpr(), TemporaryTernary));
4444
}
@@ -103,26 +103,17 @@ void DanglingHandleCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
103103
void DanglingHandleCheck::registerMatchersForVariables(MatchFinder *Finder) {
104104
const auto ConvertedHandle = handleFromTemporaryValue(IsAHandle);
105105

106-
// Find 'Handle foo(ReturnsAValue());'
106+
// Find 'Handle foo(ReturnsAValue());', 'Handle foo = ReturnsAValue();'
107107
Finder->addMatcher(
108108
varDecl(hasType(hasUnqualifiedDesugaredType(
109109
recordType(hasDeclaration(cxxRecordDecl(IsAHandle))))),
110+
unless(parmVarDecl()),
110111
hasInitializer(
111-
exprWithCleanups(has(ignoringParenImpCasts(ConvertedHandle)))
112+
exprWithCleanups(ignoringElidableConstructorCall(has(
113+
ignoringParenImpCasts(ConvertedHandle))))
112114
.bind("bad_stmt"))),
113115
this);
114116

115-
// Find 'Handle foo = ReturnsAValue();'
116-
Finder->addMatcher(
117-
traverse(TK_AsIs,
118-
varDecl(hasType(hasUnqualifiedDesugaredType(recordType(
119-
hasDeclaration(cxxRecordDecl(IsAHandle))))),
120-
unless(parmVarDecl()),
121-
hasInitializer(exprWithCleanups(
122-
has(ignoringParenImpCasts(handleFrom(
123-
IsAHandle, ConvertedHandle))))
124-
.bind("bad_stmt")))),
125-
this);
126117
// Find 'foo = ReturnsAValue(); // foo is Handle'
127118
Finder->addMatcher(
128119
traverse(TK_AsIs,
@@ -141,36 +132,35 @@ void DanglingHandleCheck::registerMatchersForVariables(MatchFinder *Finder) {
141132
void DanglingHandleCheck::registerMatchersForReturn(MatchFinder *Finder) {
142133
// Return a local.
143134
Finder->addMatcher(
144-
traverse(
145-
TK_AsIs,
146-
returnStmt(
147-
// The AST contains two constructor calls:
148-
// 1. Value to Handle conversion.
149-
// 2. Handle copy construction.
150-
// We have to match both.
151-
has(ignoringImplicit(handleFrom(
152-
IsAHandle,
153-
handleFrom(IsAHandle,
154-
declRefExpr(to(varDecl(
155-
// Is function scope ...
156-
hasAutomaticStorageDuration(),
157-
// ... and it is a local array or Value.
158-
anyOf(hasType(arrayType()),
159-
hasType(hasUnqualifiedDesugaredType(
160-
recordType(hasDeclaration(recordDecl(
161-
unless(IsAHandle)))))))))))))),
162-
// Temporary fix for false positives inside lambdas.
163-
unless(hasAncestor(lambdaExpr())))
164-
.bind("bad_stmt")),
135+
traverse(TK_AsIs,
136+
returnStmt(
137+
// The AST contains two constructor calls:
138+
// 1. Value to Handle conversion.
139+
// 2. Handle copy construction (elided in C++17+).
140+
// We have to match both.
141+
has(ignoringImplicit(ignoringElidableConstructorCall(
142+
ignoringImplicit(handleFrom(
143+
IsAHandle,
144+
declRefExpr(to(varDecl(
145+
// Is function scope ...
146+
hasAutomaticStorageDuration(),
147+
// ... and it is a local array or Value.
148+
anyOf(hasType(arrayType()),
149+
hasType(hasUnqualifiedDesugaredType(
150+
recordType(hasDeclaration(recordDecl(
151+
unless(IsAHandle))))))))))))))),
152+
// Temporary fix for false positives inside lambdas.
153+
unless(hasAncestor(lambdaExpr())))
154+
.bind("bad_stmt")),
165155
this);
166156

167157
// Return a temporary.
168158
Finder->addMatcher(
169-
traverse(
170-
TK_AsIs,
171-
returnStmt(has(exprWithCleanups(has(ignoringParenImpCasts(handleFrom(
172-
IsAHandle, handleFromTemporaryValue(IsAHandle)))))))
173-
.bind("bad_stmt")),
159+
traverse(TK_AsIs,
160+
returnStmt(has(exprWithCleanups(ignoringElidableConstructorCall(
161+
has(ignoringParenImpCasts(
162+
handleFromTemporaryValue(IsAHandle)))))))
163+
.bind("bad_stmt")),
174164
this);
175165
}
176166

‎clang-tools-extra/test/clang-tidy/checkers/bugprone/dangling-handle.cpp

+16-7
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,12 @@
1-
// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-dangling-handle %t -- \
1+
// RUN: %check_clang_tidy -std=c++11,c++14 -check-suffix=,CXX14 %s bugprone-dangling-handle %t -- \
2+
// RUN: -config="{CheckOptions: \
3+
// RUN: {bugprone-dangling-handle.HandleClasses: \
4+
// RUN: 'std::basic_string_view; ::llvm::StringRef;'}}"
5+
6+
// RUN: %check_clang_tidy -std=c++17-or-later -check-suffix=,CXX17 %s bugprone-dangling-handle %t -- \
27
// RUN: -config="{CheckOptions: \
38
// RUN: {bugprone-dangling-handle.HandleClasses: \
49
// RUN: 'std::basic_string_view; ::llvm::StringRef;'}}"
5-
// FIXME: Fix the checker to work in C++17 or later mode.
610

711
namespace std {
812

@@ -84,27 +88,32 @@ std::string ReturnsAString();
8488

8589
void Positives() {
8690
std::string_view view1 = std::string();
87-
// CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives its value [bugprone-dangling-handle]
91+
// CHECK-MESSAGES-CXX14: [[@LINE-1]]:20: warning: std::basic_string_view outlives its value [bugprone-dangling-handle]
92+
// CHECK-MESSAGES-CXX17: [[@LINE-2]]:28: warning: std::basic_string_view outlives its value [bugprone-dangling-handle]
8893

8994
std::string_view view_2 = ReturnsAString();
90-
// CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives
95+
// CHECK-MESSAGES-CXX14: [[@LINE-1]]:20: warning: std::basic_string_view outlives its value [bugprone-dangling-handle]
96+
// CHECK-MESSAGES-CXX17: [[@LINE-2]]:29: warning: std::basic_string_view outlives its value [bugprone-dangling-handle]
9197

9298
view1 = std::string();
9399
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: std::basic_string_view outlives
94100

95101
const std::string& str_ref = "";
96102
std::string_view view3 = true ? "A" : str_ref;
97-
// CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives
103+
// CHECK-MESSAGES-CXX14: [[@LINE-1]]:20: warning: std::basic_string_view outlives
104+
// CHECK-MESSAGES-CXX17: [[@LINE-2]]:28: warning: std::basic_string_view outlives
98105
view3 = true ? "A" : str_ref;
99106
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: std::basic_string_view outlives
100107

101108
std::string_view view4(ReturnsAString());
102-
// CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives
109+
// CHECK-MESSAGES-CXX14: [[@LINE-1]]:20: warning: std::basic_string_view outlives
110+
// CHECK-MESSAGES-CXX17: [[@LINE-2]]:26: warning: std::basic_string_view outlives
103111
}
104112

105113
void OtherTypes() {
106114
llvm::StringRef ref = std::string();
107-
// CHECK-MESSAGES: [[@LINE-1]]:19: warning: llvm::StringRef outlives its value
115+
// CHECK-MESSAGES-CXX14: [[@LINE-1]]:19: warning: llvm::StringRef outlives its value
116+
// CHECK-MESSAGES-CXX17: [[@LINE-2]]:25: warning: llvm::StringRef outlives its value
108117
}
109118

110119
const char static_array[] = "A";

0 commit comments

Comments
 (0)