[clang-tools-extra] f2e5000 - [clang-tidy] Fix DanglingHandleCheck to work in C++17 and later mode
Piotr Zegar via cfe-commits
cfe-commits at lists.llvm.org
Sun Sep 10 05:54:00 PDT 2023
Author: Ignat Loskutov
Date: 2023-09-10T12:52:47Z
New Revision: f2e5000937235aa35a9ee4423045b265c2c79e85
URL: https://github.com/llvm/llvm-project/commit/f2e5000937235aa35a9ee4423045b265c2c79e85
DIFF: https://github.com/llvm/llvm-project/commit/f2e5000937235aa35a9ee4423045b265c2c79e85.diff
LOG: [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
Added:
Modified:
clang-tools-extra/clang-tidy/bugprone/DanglingHandleCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone/dangling-handle.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/bugprone/DanglingHandleCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/DanglingHandleCheck.cpp
index 82c07bdc2ba95c6..9ded699ba78e66b 100644
--- a/clang-tools-extra/clang-tidy/bugprone/DanglingHandleCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/DanglingHandleCheck.cpp
@@ -36,9 +36,9 @@ ast_matchers::internal::Matcher<Stmt> handleFromTemporaryValue(
// If a ternary operator returns a temporary value, then both branches hold a
// 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()));
+ const auto TemporaryTernary = conditionalOperator(
+ hasTrueExpression(ignoringParenImpCasts(cxxBindTemporaryExpr())),
+ hasFalseExpression(ignoringParenImpCasts(cxxBindTemporaryExpr())));
return handleFrom(IsAHandle, anyOf(cxxBindTemporaryExpr(), TemporaryTernary));
}
@@ -103,26 +103,17 @@ void DanglingHandleCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
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,
@@ -141,36 +132,35 @@ void DanglingHandleCheck::registerMatchersForVariables(MatchFinder *Finder) {
void DanglingHandleCheck::registerMatchersForReturn(MatchFinder *Finder) {
// Return a local.
Finder->addMatcher(
- traverse(
- TK_AsIs,
- returnStmt(
- // The AST contains two constructor calls:
- // 1. Value to Handle conversion.
- // 2. Handle copy construction.
- // We have to match both.
- has(ignoringImplicit(handleFrom(
- IsAHandle,
- handleFrom(IsAHandle,
- declRefExpr(to(varDecl(
- // Is function scope ...
- hasAutomaticStorageDuration(),
- // ... and it is a local array or Value.
- anyOf(hasType(arrayType()),
- hasType(hasUnqualifiedDesugaredType(
- recordType(hasDeclaration(recordDecl(
- unless(IsAHandle)))))))))))))),
- // Temporary fix for false positives inside lambdas.
- unless(hasAncestor(lambdaExpr())))
- .bind("bad_stmt")),
+ traverse(TK_AsIs,
+ returnStmt(
+ // The AST contains two constructor calls:
+ // 1. Value to Handle conversion.
+ // 2. Handle copy construction (elided in C++17+).
+ // We have to match both.
+ has(ignoringImplicit(ignoringElidableConstructorCall(
+ ignoringImplicit(handleFrom(
+ IsAHandle,
+ declRefExpr(to(varDecl(
+ // Is function scope ...
+ hasAutomaticStorageDuration(),
+ // ... and it is a local array or Value.
+ anyOf(hasType(arrayType()),
+ hasType(hasUnqualifiedDesugaredType(
+ recordType(hasDeclaration(recordDecl(
+ unless(IsAHandle))))))))))))))),
+ // Temporary fix for false positives inside lambdas.
+ unless(hasAncestor(lambdaExpr())))
+ .bind("bad_stmt")),
this);
// Return a temporary.
Finder->addMatcher(
- traverse(
- TK_AsIs,
- returnStmt(has(exprWithCleanups(has(ignoringParenImpCasts(handleFrom(
- IsAHandle, handleFromTemporaryValue(IsAHandle)))))))
- .bind("bad_stmt")),
+ traverse(TK_AsIs,
+ returnStmt(has(exprWithCleanups(ignoringElidableConstructorCall(
+ has(ignoringParenImpCasts(
+ handleFromTemporaryValue(IsAHandle)))))))
+ .bind("bad_stmt")),
this);
}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/dangling-handle.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/dangling-handle.cpp
index e99cc8b99d6a420..23cda5321764383 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/dangling-handle.cpp
+++ b/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 @@ std::string ReturnsAString();
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";
More information about the cfe-commits
mailing list