diff --git a/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrActions.cpp b/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrActions.cpp index 79b8b5fc1b49..53c6341b31cb 100644 --- a/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrActions.cpp +++ b/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrActions.cpp @@ -69,21 +69,25 @@ llvm::StringRef GetOutermostMacroName( } } -/// \brief Looks for a sequences of 0 or more explicit casts with an implicit -/// null-to-pointer cast within. +/// \brief Looks for implicit casts as well as sequences of 0 or more explicit +/// casts with an implicit null-to-pointer cast within. /// -/// The matcher this visitor is used with will find a top-most explicit cast -/// (i.e. it has no explicit casts as an ancestor) where an implicit cast is -/// nested within. However, there is no guarantee that only explicit casts -/// exist between the found top-most explicit cast and the possibly more than -/// one nested implicit cast. This visitor finds all cast sequences with an -/// implicit cast to null within and creates a replacement leaving the -/// outermost explicit cast unchanged to avoid introducing ambiguities. +/// The matcher this visitor is used with will find a single implicit cast or a +/// top-most explicit cast (i.e. it has no explicit casts as an ancestor) where +/// an implicit cast is nested within. However, there is no guarantee that only +/// explicit casts exist between the found top-most explicit cast and the +/// possibly more than one nested implicit cast. This visitor finds all cast +/// sequences with an implicit cast to null within and creates a replacement +/// leaving the outermost explicit cast unchanged to avoid introducing +/// ambiguities. class CastSequenceVisitor : public RecursiveASTVisitor { public: CastSequenceVisitor(tooling::Replacements &R, SourceManager &SM, + const LangOptions &LangOpts, + const UserMacroNames &UserNullMacros, unsigned &AcceptedChanges) - : Replace(R), SM(SM), AcceptedChanges(AcceptedChanges), FirstSubExpr(0) {} + : Replace(R), SM(SM), LangOpts(LangOpts), UserNullMacros(UserNullMacros), + AcceptedChanges(AcceptedChanges), FirstSubExpr(0) {} // Only VisitStmt is overridden as we shouldn't find other base AST types // within a cast expression. @@ -94,8 +98,14 @@ public: ResetFirstSubExpr(); return true; } else if (!FirstSubExpr) { - // Get the subexpression of the outermost explicit cast - FirstSubExpr = C->getSubExpr(); + // Keep parentheses for implicit casts to avoid cases where an implicit + // cast within a parentheses expression is right next to a return + // statement otherwise get the subexpression of the outermost explicit + // cast. + if (C->getStmtClass() == Stmt::ImplicitCastExprClass) + FirstSubExpr = C->IgnoreParenImpCasts(); + else + FirstSubExpr = C->getSubExpr(); } if (C->getCastKind() == CK_NullToPointer || @@ -104,9 +114,28 @@ public: SourceLocation StartLoc = FirstSubExpr->getLocStart(); SourceLocation EndLoc = FirstSubExpr->getLocEnd(); - // If the start/end location is a macro, get the expansion location. - StartLoc = SM.getFileLoc(StartLoc); - EndLoc = SM.getFileLoc(EndLoc); + // If the start/end location is a macro argument expansion, get the + // expansion location. If its a macro body expansion, check to see if its + // coming from a macro called NULL. + if (SM.isMacroArgExpansion(StartLoc) && SM.isMacroArgExpansion(EndLoc)) { + StartLoc = SM.getFileLoc(StartLoc); + EndLoc = SM.getFileLoc(EndLoc); + } else if (SM.isMacroBodyExpansion(StartLoc) && + SM.isMacroBodyExpansion(EndLoc)) { + llvm::StringRef OutermostMacroName = + GetOutermostMacroName(StartLoc, SM, LangOpts); + + // Check to see if the user wants to replace the macro being expanded. + bool ReplaceNullMacro = + std::find(UserNullMacros.begin(), UserNullMacros.end(), + OutermostMacroName) != UserNullMacros.end(); + + if (!ReplaceNullMacro) + return false; + + StartLoc = SM.getFileLoc(StartLoc); + EndLoc = SM.getFileLoc(EndLoc); + } AcceptedChanges += ReplaceWithNullptr(Replace, SM, StartLoc, EndLoc) ? 1 : 0; @@ -123,6 +152,8 @@ private: private: tooling::Replacements &Replace; SourceManager &SM; + const LangOptions &LangOpts; + const UserMacroNames &UserNullMacros; unsigned &AcceptedChanges; Expr *FirstSubExpr; }; @@ -141,44 +172,11 @@ void NullptrFixer::run(const ast_matchers::MatchFinder::MatchResult &Result) { SourceManager &SM = *Result.SourceManager; const CastExpr *NullCast = Result.Nodes.getNodeAs(CastSequence); - if (NullCast) { - // Given an explicit cast with an implicit null-to-pointer cast within - // use CastSequenceVisitor to identify sequences of explicit casts that can - // be converted into 'nullptr'. - CastSequenceVisitor Visitor(Replace, SM, AcceptedChanges); - Visitor.TraverseStmt(const_cast(NullCast)); - } - - const CastExpr *Cast = Result.Nodes.getNodeAs(ImplicitCastNode); - if (Cast) { - const Expr *E = Cast->IgnoreParenImpCasts(); - SourceLocation StartLoc = E->getLocStart(); - SourceLocation EndLoc = E->getLocEnd(); - - // If the start/end location is a macro argument expansion, get the - // expansion location. If its a macro body expansion, check to see if its - // coming from a macro called NULL. - if (SM.isMacroArgExpansion(StartLoc) && SM.isMacroArgExpansion(EndLoc)) { - StartLoc = SM.getFileLoc(StartLoc); - EndLoc = SM.getFileLoc(EndLoc); - } else if (SM.isMacroBodyExpansion(StartLoc) && - SM.isMacroBodyExpansion(EndLoc)) { - llvm::StringRef OutermostMacroName = - GetOutermostMacroName(StartLoc, SM, Result.Context->getLangOpts()); - - // Check to see if the user wants to replace the macro being expanded. - bool ReplaceNullMacro = - std::find(UserNullMacros.begin(), UserNullMacros.end(), - OutermostMacroName) != UserNullMacros.end(); - - if (!ReplaceNullMacro) - return; - - StartLoc = SM.getFileLoc(StartLoc); - EndLoc = SM.getFileLoc(EndLoc); - } - - AcceptedChanges += - ReplaceWithNullptr(Replace, SM, StartLoc, EndLoc) ? 1 : 0; - } + assert(NullCast && "Bad Callback. No node provided"); + // Given an implicit null-ptr cast or an explicit cast with an implicit + // null-to-pointer cast within use CastSequenceVisitor to identify sequences + // of explicit casts that can be converted into 'nullptr'. + CastSequenceVisitor Visitor(Replace, SM, Result.Context->getLangOpts(), + UserNullMacros, AcceptedChanges); + Visitor.TraverseStmt(const_cast(NullCast)); } diff --git a/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrActions.h b/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrActions.h index a79cb301543c..9835a5375e20 100644 --- a/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrActions.h +++ b/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrActions.h @@ -19,6 +19,9 @@ #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Tooling/Refactoring.h" +// The type for user-defined macro names that behave like NULL +typedef llvm::SmallVector UserMacroNames; + /// \brief The callback to be used for nullptr migration matchers. /// class NullptrFixer : public clang::ast_matchers::MatchFinder::MatchCallback { @@ -33,7 +36,7 @@ public: private: clang::tooling::Replacements &Replace; unsigned &AcceptedChanges; - llvm::SmallVector UserNullMacros; + UserMacroNames UserNullMacros; }; #endif // LLVM_TOOLS_CLANG_TOOLS_EXTRA_CPP11_MIGRATE_NULLPTR_ACTIONS_H diff --git a/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrMatchers.cpp b/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrMatchers.cpp index 86e00c189d55..e4baf05432f5 100644 --- a/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrMatchers.cpp +++ b/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrMatchers.cpp @@ -18,7 +18,6 @@ using namespace clang::ast_matchers; using namespace clang; -const char *ImplicitCastNode = "cast"; const char *CastSequence = "sequence"; namespace clang { @@ -47,30 +46,22 @@ AST_MATCHER(Type, sugaredNullptrType) { } // end namespace ast_matchers } // end namespace clang -StatementMatcher makeImplicitCastMatcher() { - return implicitCastExpr( - isNullToPointer(), - unless(hasAncestor(explicitCastExpr())), - unless( - hasSourceExpression( - hasType(sugaredNullptrType()) - ) - ) - ).bind(ImplicitCastNode); -} - StatementMatcher makeCastSequenceMatcher() { - return explicitCastExpr( + StatementMatcher ImplicitCastToNull = + implicitCastExpr( + isNullToPointer(), + unless( + hasSourceExpression( + hasType(sugaredNullptrType()) + ) + ) + ); + + return castExpr( unless(hasAncestor(explicitCastExpr())), - hasDescendant( - implicitCastExpr( - isNullToPointer(), - unless( - hasSourceExpression( - hasType(sugaredNullptrType()) - ) - ) - ) + anyOf( + hasDescendant(ImplicitCastToNull), + ImplicitCastToNull ) ).bind(CastSequence); } diff --git a/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrMatchers.h b/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrMatchers.h index e6d231754807..bb10e54bd36a 100644 --- a/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrMatchers.h +++ b/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrMatchers.h @@ -18,21 +18,13 @@ #include "clang/ASTMatchers/ASTMatchers.h" // Names to bind with matched expressions. -extern const char *ImplicitCastNode; extern const char *CastSequence; -/// \brief Create a matcher to find the implicit casts clang inserts -/// when writing null to a pointer. -/// -/// However, don't match those implicit casts that have explicit casts as -/// an ancestor. Explicit casts are handled by makeCastSequenceMatcher(). -clang::ast_matchers::StatementMatcher makeImplicitCastMatcher(); - -/// \brief Create a matcher that finds the head of a sequence of nested explicit -/// casts that have an implicit cast to null within. -/// -/// This matcher is necessary so that an entire sequence of explicit casts can -/// be replaced instead of just the inner-most implicit cast. +/// \brief Create a matcher that finds implicit casts as well as the head of a +/// sequence of zero or more nested explicit casts that have an implicit cast +/// to null within. +/// Finding sequences of explict casts is necessary so that an entire sequence +/// can be replaced instead of just the inner-most implicit cast. clang::ast_matchers::StatementMatcher makeCastSequenceMatcher(); #endif // LLVM_TOOLS_CLANG_TOOLS_EXTRA_CPP11_MIGRATE_USE_NULLPTR_MATCHERS_H diff --git a/clang-tools-extra/cpp11-migrate/UseNullptr/UseNullptr.cpp b/clang-tools-extra/cpp11-migrate/UseNullptr/UseNullptr.cpp index fe90cfcccb40..573604a9c531 100644 --- a/clang-tools-extra/cpp11-migrate/UseNullptr/UseNullptr.cpp +++ b/clang-tools-extra/cpp11-migrate/UseNullptr/UseNullptr.cpp @@ -45,7 +45,6 @@ int UseNullptrTransform::apply(const FileContentsByPath &InputStates, AcceptedChanges, MaxRisk); - Finder.addMatcher(makeImplicitCastMatcher(), &Fixer); Finder.addMatcher(makeCastSequenceMatcher(), &Fixer); if (int result = UseNullptrTool.run(newFrontendActionFactory(&Finder))) { diff --git a/clang-tools-extra/test/cpp11-migrate/UseNullptr/macros.cpp b/clang-tools-extra/test/cpp11-migrate/UseNullptr/macros.cpp index 28de7458c601..8e19e4ab6dcf 100644 --- a/clang-tools-extra/test/cpp11-migrate/UseNullptr/macros.cpp +++ b/clang-tools-extra/test/cpp11-migrate/UseNullptr/macros.cpp @@ -39,7 +39,21 @@ void test_macro_expansion1() { #undef MACRO_EXPANSION_HAS_NULL } +// Test macro expansion with cast sequence, PR15572 void test_macro_expansion2() { +#define MACRO_EXPANSION_HAS_NULL \ + dummy((int*)0); \ + side_effect(); + // CHECK: dummy((int*)0); \ + // CHECK-NEXT: side_effect(); + + MACRO_EXPANSION_HAS_NULL; + // CHECK: MACRO_EXPANSION_HAS_NULL; + +#undef MACRO_EXPANSION_HAS_NULL +} + +void test_macro_expansion3() { #define MACRO_EXPANSION_HAS_NULL \ dummy(NULL); \ side_effect(); @@ -57,7 +71,7 @@ void test_macro_expansion2() { #undef MACRO_EXPANSION_HAS_NULL } -void test_macro_expansion3() { +void test_macro_expansion4() { #define MY_NULL NULL int *p = MY_NULL; // CHECK: int *p = MY_NULL; @@ -91,5 +105,3 @@ void test_function_like_macro2() { // CHECK: my_macro(p != nullptr); #undef my_macro } - -