loskutov created this revision.
loskutov added reviewers: sbenza, PiotrZSL.
loskutov added a project: clang-tools-extra.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a reviewer: njames93.
Herald added a project: All.
loskutov requested review of this revision.
Herald added a subscriber: cfe-commits.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D158371
Files:
clang-tools-extra/clang-tidy/bugprone/DanglingHandleCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone/dangling-handle.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/dangling-handle.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/dangling-handle.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/dangling-handle.cpp
@@ -1,8 +1,12 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-dangling-handle %t -- \
+// RUN: %check_clang_tidy -std=c++11,c++14 -check-suffix=,CXX14 %s bugprone-dangling-handle %t -- \
+// RUN: -config="{CheckOptions: \
+// RUN: {bugprone-dangling-handle.HandleClasses: \
+// RUN: 'std::basic_string_view; ::llvm::StringRef;'}}"
+
+// RUN: %check_clang_tidy -std=c++17-or-later -check-suffix=,CXX17 %s bugprone-dangling-handle %t -- \
// RUN: -config="{CheckOptions: \
// RUN: {bugprone-dangling-handle.HandleClasses: \
// RUN: 'std::basic_string_view; ::llvm::StringRef;'}}"
-// FIXME: Fix the checker to work in C++17 or later mode.
namespace std {
@@ -84,27 +88,32 @@
void Positives() {
std::string_view view1 = std::string();
- // CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives its value [bugprone-dangling-handle]
+ // CHECK-MESSAGES-CXX14: [[@LINE-1]]:20: warning: std::basic_string_view outlives its value [bugprone-dangling-handle]
+ // CHECK-MESSAGES-CXX17: [[@LINE-2]]:28: warning: std::basic_string_view outlives its value [bugprone-dangling-handle]
std::string_view view_2 = ReturnsAString();
- // CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives
+ // CHECK-MESSAGES-CXX14: [[@LINE-1]]:20: warning: std::basic_string_view outlives its value [bugprone-dangling-handle]
+ // CHECK-MESSAGES-CXX17: [[@LINE-2]]:29: warning: std::basic_string_view outlives its value [bugprone-dangling-handle]
view1 = std::string();
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: std::basic_string_view outlives
const std::string& str_ref = "";
std::string_view view3 = true ? "A" : str_ref;
- // CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives
+ // CHECK-MESSAGES-CXX14: [[@LINE-1]]:20: warning: std::basic_string_view outlives
+ // CHECK-MESSAGES-CXX17: [[@LINE-2]]:28: warning: std::basic_string_view outlives
view3 = true ? "A" : str_ref;
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: std::basic_string_view outlives
std::string_view view4(ReturnsAString());
- // CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives
+ // CHECK-MESSAGES-CXX14: [[@LINE-1]]:20: warning: std::basic_string_view outlives
+ // CHECK-MESSAGES-CXX17: [[@LINE-2]]:26: warning: std::basic_string_view outlives
}
void OtherTypes() {
llvm::StringRef ref = std::string();
- // CHECK-MESSAGES: [[@LINE-1]]:19: warning: llvm::StringRef outlives its value
+ // CHECK-MESSAGES-CXX14: [[@LINE-1]]:19: warning: llvm::StringRef outlives its value
+ // CHECK-MESSAGES-CXX17: [[@LINE-2]]:25: warning: llvm::StringRef outlives its value
}
const char static_array[] = "A";
Index: clang-tools-extra/clang-tidy/bugprone/DanglingHandleCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/DanglingHandleCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/DanglingHandleCheck.cpp
@@ -37,8 +37,8 @@
// temporary value. If one of them is not a temporary then it must be copied
// into one to satisfy the type of the operator.
const auto TemporaryTernary =
- conditionalOperator(hasTrueExpression(cxxBindTemporaryExpr()),
- hasFalseExpression(cxxBindTemporaryExpr()));
+ conditionalOperator(hasTrueExpression(ignoringParenImpCasts(cxxBindTemporaryExpr())),
+ hasFalseExpression(ignoringParenImpCasts(cxxBindTemporaryExpr())));
return handleFrom(IsAHandle, anyOf(cxxBindTemporaryExpr(), TemporaryTernary));
}
@@ -103,26 +103,17 @@
void DanglingHandleCheck::registerMatchersForVariables(MatchFinder *Finder) {
const auto ConvertedHandle = handleFromTemporaryValue(IsAHandle);
- // Find 'Handle foo(ReturnsAValue());'
+ // Find 'Handle foo(ReturnsAValue());', 'Handle foo = ReturnsAValue();'
Finder->addMatcher(
varDecl(hasType(hasUnqualifiedDesugaredType(
recordType(hasDeclaration(cxxRecordDecl(IsAHandle))))),
+ unless(parmVarDecl()),
hasInitializer(
- exprWithCleanups(has(ignoringParenImpCasts(ConvertedHandle)))
+ exprWithCleanups(ignoringElidableConstructorCall(has(
+ ignoringParenImpCasts(ConvertedHandle))))
.bind("bad_stmt"))),
this);
- // Find 'Handle foo = ReturnsAValue();'
- Finder->addMatcher(
- traverse(TK_AsIs,
- varDecl(hasType(hasUnqualifiedDesugaredType(recordType(
- hasDeclaration(cxxRecordDecl(IsAHandle))))),
- unless(parmVarDecl()),
- hasInitializer(exprWithCleanups(
- has(ignoringParenImpCasts(handleFrom(
- IsAHandle, ConvertedHandle))))
- .bind("bad_stmt")))),
- this);
// Find 'foo = ReturnsAValue(); // foo is Handle'
Finder->addMatcher(
traverse(TK_AsIs,
@@ -146,10 +137,9 @@
returnStmt(
// The AST contains two constructor calls:
// 1. Value to Handle conversion.
- // 2. Handle copy construction.
+ // 2. Handle copy construction (elided in C++17+).
// We have to match both.
- has(ignoringImplicit(handleFrom(
- IsAHandle,
+ has(ignoringImplicit(ignoringElidableConstructorCall(ignoringImplicit(
handleFrom(IsAHandle,
declRefExpr(to(varDecl(
// Is function scope ...
@@ -158,7 +148,7 @@
anyOf(hasType(arrayType()),
hasType(hasUnqualifiedDesugaredType(
recordType(hasDeclaration(recordDecl(
- unless(IsAHandle)))))))))))))),
+ unless(IsAHandle))))))))))))))),
// Temporary fix for false positives inside lambdas.
unless(hasAncestor(lambdaExpr())))
.bind("bad_stmt")),
@@ -168,8 +158,9 @@
Finder->addMatcher(
traverse(
TK_AsIs,
- returnStmt(has(exprWithCleanups(has(ignoringParenImpCasts(handleFrom(
- IsAHandle, handleFromTemporaryValue(IsAHandle)))))))
+ returnStmt(has(exprWithCleanups(ignoringElidableConstructorCall(
+ has(ignoringParenImpCasts(
+ handleFromTemporaryValue(IsAHandle)))))))
.bind("bad_stmt")),
this);
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
