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"; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
