[clang-tools-extra] r181221 - Added comprehensive macro arg usage logic to Use-Nullptr Transform
Edwin Vane
edwin.vane at intel.com
Mon May 6 10:27:02 PDT 2013
Author: revane
Date: Mon May 6 12:27:02 2013
New Revision: 181221
URL: http://llvm.org/viewvc/llvm-project?rev=181221&view=rev
Log:
Added comprehensive macro arg usage logic to Use-Nullptr Transform
If a NullTo(Member)Pointer cast results from a macro arg expansion, all uses of
the arg must result in such casts or else the macro arg cannot be replaced with
'nullptr' safely. This revision adds logic and tests to ensure the safety of
replacing macro args in the Use-Nullptr Transform.
Fixes: PR15816
Modified:
clang-tools-extra/trunk/cpp11-migrate/UseNullptr/NullptrActions.cpp
clang-tools-extra/trunk/test/cpp11-migrate/UseNullptr/macros.cpp
Modified: clang-tools-extra/trunk/cpp11-migrate/UseNullptr/NullptrActions.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/cpp11-migrate/UseNullptr/NullptrActions.cpp?rev=181221&r1=181220&r2=181221&view=diff
==============================================================================
--- clang-tools-extra/trunk/cpp11-migrate/UseNullptr/NullptrActions.cpp (original)
+++ clang-tools-extra/trunk/cpp11-migrate/UseNullptr/NullptrActions.cpp Mon May 6 12:27:02 2013
@@ -36,24 +36,25 @@ static llvm::cl::opt<std::string> UserNu
"macro names that behave like NULL"),
llvm::cl::init(""));
+bool isReplaceableRange(SourceLocation StartLoc, SourceLocation EndLoc,
+ const SourceManager &SM) {
+ return SM.isFromSameFile(StartLoc, EndLoc) && SM.isFromMainFile(StartLoc);
+}
+
/// \brief Replaces the provided range with the text "nullptr", but only if
/// the start and end location are both in main file.
/// Returns true if and only if a replacement was made.
-bool ReplaceWithNullptr(tooling::Replacements &Replace, SourceManager &SM,
+void ReplaceWithNullptr(tooling::Replacements &Replace, SourceManager &SM,
SourceLocation StartLoc, SourceLocation EndLoc) {
- if (SM.isFromSameFile(StartLoc, EndLoc) && SM.isFromMainFile(StartLoc)) {
- CharSourceRange Range(SourceRange(StartLoc, EndLoc), true);
- // Add a space if nullptr follows an alphanumeric character. This happens
- // whenever there is an c-style explicit cast to nullptr not surrounded by
- // parentheses and right beside a return statement.
- SourceLocation PreviousLocation = StartLoc.getLocWithOffset(-1);
- if (isAlphanumeric(*FullSourceLoc(PreviousLocation, SM).getCharacterData()))
- Replace.insert(tooling::Replacement(SM, Range, " nullptr"));
- else
- Replace.insert(tooling::Replacement(SM, Range, "nullptr"));
- return true;
- } else
- return false;
+ CharSourceRange Range(SourceRange(StartLoc, EndLoc), true);
+ // Add a space if nullptr follows an alphanumeric character. This happens
+ // whenever there is an c-style explicit cast to nullptr not surrounded by
+ // parentheses and right beside a return statement.
+ SourceLocation PreviousLocation = StartLoc.getLocWithOffset(-1);
+ if (isAlphanumeric(*FullSourceLoc(PreviousLocation, SM).getCharacterData()))
+ Replace.insert(tooling::Replacement(SM, Range, " nullptr"));
+ else
+ Replace.insert(tooling::Replacement(SM, Range, "nullptr"));
}
/// \brief Returns the name of the outermost macro.
@@ -75,6 +76,245 @@ llvm::StringRef GetOutermostMacroName(
return clang::Lexer::getImmediateMacroName(OutermostMacroLoc, SM, LO);
}
+
+/// \brief Given the SourceLocation for a macro arg expansion, finds the
+/// non-macro SourceLocation of the macro the arg was passed to and the
+/// non-macro SourceLocation of the argument in the arg list to that macro.
+/// These results are returned via \c MacroLoc and \c ArgLoc respectively.
+/// These values are undefined if the return value is false.
+///
+/// \returns false if one of the returned SourceLocations would be a
+/// SourceLocation pointing within the definition of another macro.
+bool getMacroAndArgLocations(SourceLocation Loc, const SourceManager &SM,
+ SourceLocation &ArgLoc, SourceLocation &MacroLoc) {
+ assert(Loc.isMacroID() && "Only reasonble to call this on macros");
+
+ ArgLoc = Loc;
+
+ // Find the location of the immediate macro expansion.
+ while (1) {
+ std::pair<FileID, unsigned> LocInfo = SM.getDecomposedLoc(ArgLoc);
+ const SrcMgr::SLocEntry *E = &SM.getSLocEntry(LocInfo.first);
+ const SrcMgr::ExpansionInfo &Expansion = E->getExpansion();
+
+ ArgLoc = Expansion.getExpansionLocStart();
+ if (!Expansion.isMacroArgExpansion()) {
+ // TODO: Insert test for user-defined null macro here.
+ return MacroLoc.isFileID();
+ }
+
+ MacroLoc = SM.getImmediateExpansionRange(ArgLoc).first;
+
+ ArgLoc = Expansion.getSpellingLoc().getLocWithOffset(LocInfo.second);
+ if (ArgLoc.isFileID())
+ return true;
+
+ // If spelling location resides in the same FileID as macro expansion
+ // location, it means there is no inner macro.
+ FileID MacroFID = SM.getFileID(MacroLoc);
+ if (SM.isInFileID(ArgLoc, MacroFID))
+ // Don't transform this case. If the characters that caused the
+ // null-conversion come from within a macro, they can't be changed.
+ return false;
+ }
+
+ llvm_unreachable("getMacroAndArgLocations");
+}
+
+/// \brief Tests if TestMacroLoc is found while recursively unravelling
+/// expansions starting at TestLoc. TestMacroLoc.isFileID() must be true.
+/// Implementation is very similar to getMacroAndArgLocations() except in this
+/// case, it's not assumed that TestLoc is expanded from a macro argument.
+/// While unravelling expansions macro arguments are handled as with
+/// getMacroAndArgLocations() but in this function macro body expansions are
+/// also handled.
+///
+/// False means either:
+/// - TestLoc is not from a macro expansion
+/// - TestLoc is from a different macro expansion
+bool expandsFrom(SourceLocation TestLoc, SourceLocation TestMacroLoc,
+ const SourceManager &SM) {
+ if (TestLoc.isFileID()) {
+ return false;
+ }
+
+ SourceLocation Loc = TestLoc, MacroLoc;
+
+ while (1) {
+ std::pair<FileID, unsigned> LocInfo = SM.getDecomposedLoc(Loc);
+ const SrcMgr::SLocEntry *E = &SM.getSLocEntry(LocInfo.first);
+ const SrcMgr::ExpansionInfo &Expansion = E->getExpansion();
+
+ Loc = Expansion.getExpansionLocStart();
+
+ if (!Expansion.isMacroArgExpansion()) {
+ if (Loc.isFileID()) {
+ if (Loc == TestMacroLoc)
+ // Match made.
+ return true;
+ return false;
+ }
+ // Since Loc is still a macro ID and it's not an argument expansion, we
+ // don't need to do the work of handling an argument expansion. Simply
+ // keep recursively expanding until we hit a FileID or a macro arg
+ // expansion or a macro arg expansion.
+ continue;
+ }
+
+ MacroLoc = SM.getImmediateExpansionRange(Loc).first;
+ if (MacroLoc.isFileID() && MacroLoc == TestMacroLoc)
+ // Match made.
+ return true;
+
+ Loc = Expansion.getSpellingLoc();
+ Loc = Expansion.getSpellingLoc().getLocWithOffset(LocInfo.second);
+ if (Loc.isFileID())
+ // If we made it this far without finding a match, there is no match to
+ // be made.
+ return false;
+ }
+
+ llvm_unreachable("expandsFrom");
+}
+
+/// \brief Given a starting point \c Start in the AST, find an ancestor that
+/// doesn't expand from the macro called at file location \c MacroLoc.
+///
+/// \pre MacroLoc.isFileID()
+/// \returns true if such an ancestor was found, false otherwise.
+bool findContainingAncestor(ast_type_traits::DynTypedNode Start,
+ SourceLocation MacroLoc,
+ ast_type_traits::DynTypedNode &Result,
+ ASTContext &Context) {
+ // Below we're only following the first parent back up the AST. This should
+ // be fine since for the statements we care about there should only be one
+ // parent as far up as we care. If this assumption doesn't hold, need to
+ // revisit what to do here.
+
+ assert(MacroLoc.isFileID());
+
+ do {
+ ASTContext::ParentVector Parents = Context.getParents(Start);
+ if (Parents.empty())
+ return false;
+ assert(Parents.size() == 1 &&
+ "Found an ancestor with more than one parent!");
+
+ ASTContext::ParentVector::const_iterator I = Parents.begin();
+
+ SourceLocation Loc;
+ if (const Decl *D = I->get<Decl>())
+ Loc = D->getLocStart();
+ else if (const Stmt *S = I->get<Stmt>())
+ Loc = S->getLocStart();
+ else
+ llvm_unreachable("Expected to find Decl or Stmt containing ancestor");
+
+ if (!expandsFrom(Loc, MacroLoc, Context.getSourceManager())) {
+ Result = *I;
+ return true;
+ }
+ Start = *I;
+ } while(1);
+
+ llvm_unreachable("findContainingAncestor");
+}
+
+/// \brief RecursiveASTVisitor for ensuring all nodes rooted at a given AST
+/// subtree that have file-level source locations corresponding to a macro
+/// argument have implicit NullTo(Member)Pointer nodes as ancestors.
+class MacroArgUsageVisitor : public RecursiveASTVisitor<MacroArgUsageVisitor> {
+public:
+ MacroArgUsageVisitor(SourceLocation CastLoc, const SourceManager &SM)
+ : CastLoc(CastLoc), SM(SM), Visited(false), CastFound(false),
+ InvalidFound(false) {
+ assert(CastLoc.isFileID());
+ }
+
+ bool TraverseStmt(Stmt *S) {
+ bool VisitedPreviously = Visited;
+
+ if (!RecursiveASTVisitor<MacroArgUsageVisitor>::TraverseStmt(S))
+ return false;
+
+ // The point at which VisitedPreviously is false and Visited is true is the
+ // root of a subtree containing nodes whose locations match CastLoc. It's
+ // at this point we test that the Implicit NullTo(Member)Pointer cast was
+ // found or not.
+ if (!VisitedPreviously) {
+ if (Visited && !CastFound) {
+ // Found nodes with matching SourceLocations but didn't come across a
+ // cast. This is an invalid macro arg use. Can stop traversal
+ // completely now.
+ InvalidFound = true;
+ return false;
+ }
+ // Reset state as we unwind back up the tree.
+ CastFound = false;
+ Visited = false;
+ }
+ return true;
+ }
+
+ bool VisitStmt(Stmt *S) {
+ if (SM.getFileLoc(S->getLocStart()) != CastLoc)
+ return true;
+ Visited = true;
+
+ const ImplicitCastExpr *Cast = dyn_cast<ImplicitCastExpr>(S);
+ if (Cast && (Cast->getCastKind() == CK_NullToPointer ||
+ Cast->getCastKind() == CK_NullToMemberPointer))
+ CastFound = true;
+
+ return true;
+ }
+
+ bool foundInvalid() const { return InvalidFound; }
+
+private:
+ SourceLocation CastLoc;
+ const SourceManager &SM;
+
+ bool Visited;
+ bool CastFound;
+ bool InvalidFound;
+};
+
+/// \brief Tests that all expansions of a macro arg, one of which expands to
+/// result in \p CE, yield NullTo(Member)Pointer casts.
+bool allArgUsesValid(const CastExpr *CE, ASTContext &Context) {
+ SourceLocation CastLoc = CE->getLocStart();
+ const SourceManager &SM = Context.getSourceManager();
+
+ // Step 1: Get location of macro arg and location of the macro the arg was
+ // provided to.
+ SourceLocation ArgLoc, MacroLoc;
+ if (!getMacroAndArgLocations(CastLoc, SM, ArgLoc, MacroLoc))
+ return false;
+
+ // Step 2: Find the first ancestor that doesn't expand from this macro.
+ ast_type_traits::DynTypedNode ContainingAncestor;
+ if (!findContainingAncestor(ast_type_traits::DynTypedNode::create<Stmt>(*CE),
+ MacroLoc, ContainingAncestor, Context))
+ return false;
+
+ // Step 3:
+ // Visit children of this containing parent looking for the least-descended
+ // nodes of the containing parent which are macro arg expansions that expand
+ // from the given arg location.
+ // Visitor needs: arg loc
+ MacroArgUsageVisitor ArgUsageVisitor(SM.getFileLoc(CastLoc), SM);
+ if (const Decl *D = ContainingAncestor.get<Decl>())
+ ArgUsageVisitor.TraverseDecl(const_cast<Decl*>(D));
+ else if (const Stmt *S = ContainingAncestor.get<Stmt>())
+ ArgUsageVisitor.TraverseStmt(const_cast<Stmt*>(S));
+ else
+ llvm_unreachable("Unhandled ContainingAncestor node type");
+
+ if (ArgUsageVisitor.foundInvalid())
+ return false;
+
+ return true;
}
/// \brief Looks for implicit casts as well as sequences of 0 or more explicit
@@ -90,22 +330,31 @@ llvm::StringRef GetOutermostMacroName(
/// ambiguities.
class CastSequenceVisitor : public RecursiveASTVisitor<CastSequenceVisitor> {
public:
- CastSequenceVisitor(tooling::Replacements &R, SourceManager &SM,
- const LangOptions &LangOpts,
+ CastSequenceVisitor(tooling::Replacements &R, ASTContext &Context,
const UserMacroNames &UserNullMacros,
unsigned &AcceptedChanges)
- : Replace(R), SM(SM), LangOpts(LangOpts), UserNullMacros(UserNullMacros),
- AcceptedChanges(AcceptedChanges), FirstSubExpr(0) {}
+ : Replace(R), SM(Context.getSourceManager()), Context(Context),
+ UserNullMacros(UserNullMacros), AcceptedChanges(AcceptedChanges),
+ FirstSubExpr(0), PruneSubtree(false) {}
+
+ bool TraverseStmt(Stmt *S) {
+ // Stop traversing down the tree if requested.
+ if (PruneSubtree) {
+ PruneSubtree = false;
+ return true;
+ }
+ return RecursiveASTVisitor<CastSequenceVisitor>::TraverseStmt(S);
+ }
// Only VisitStmt is overridden as we shouldn't find other base AST types
// within a cast expression.
bool VisitStmt(Stmt *S) {
CastExpr *C = dyn_cast<CastExpr>(S);
if (!C) {
- ResetFirstSubExpr();
+ FirstSubExpr = 0;
return true;
} else if (!FirstSubExpr) {
- FirstSubExpr = C->getSubExpr()->IgnoreParens();
+ FirstSubExpr = C->getSubExpr()->IgnoreParens();
}
if (C->getCastKind() == CK_NullToPointer ||
@@ -114,49 +363,62 @@ public:
SourceLocation StartLoc = FirstSubExpr->getLocStart();
SourceLocation EndLoc = FirstSubExpr->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 the location comes from a macro arg expansion, *all* uses of that
+ // arg must be checked to result in NullTo(Member)Pointer casts.
+ //
+ // If the location comes from a macro body expansion, check to see if its
+ // coming from one of the allowed 'NULL' macros.
if (SM.isMacroArgExpansion(StartLoc) && SM.isMacroArgExpansion(EndLoc)) {
- StartLoc = SM.getFileLoc(StartLoc);
- EndLoc = SM.getFileLoc(EndLoc);
- } else if (SM.isMacroBodyExpansion(StartLoc) &&
- SM.isMacroBodyExpansion(EndLoc)) {
+ SourceLocation FileLocStart = SM.getFileLoc(StartLoc),
+ FileLocEnd = SM.getFileLoc(EndLoc);
+ if (isReplaceableRange(FileLocStart, FileLocEnd, SM) &&
+ allArgUsesValid(C, Context)) {
+ ReplaceWithNullptr(Replace, SM, FileLocStart, FileLocEnd);
+ ++AcceptedChanges;
+ }
+ return skipSubTree();
+ }
+
+ if (SM.isMacroBodyExpansion(StartLoc) &&
+ SM.isMacroBodyExpansion(EndLoc)) {
llvm::StringRef OutermostMacroName =
- GetOutermostMacroName(StartLoc, SM, LangOpts);
+ GetOutermostMacroName(StartLoc, SM, 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 false;
+ if (std::find(UserNullMacros.begin(), UserNullMacros.end(),
+ OutermostMacroName) == UserNullMacros.end()) {
+ return skipSubTree();
+ }
StartLoc = SM.getFileLoc(StartLoc);
EndLoc = SM.getFileLoc(EndLoc);
}
- AcceptedChanges +=
- ReplaceWithNullptr(Replace, SM, StartLoc, EndLoc) ? 1 : 0;
+ if (!isReplaceableRange(StartLoc, EndLoc, SM)) {
+ return skipSubTree();
+ }
+ ReplaceWithNullptr(Replace, SM, StartLoc, EndLoc);
+ ++AcceptedChanges;
- ResetFirstSubExpr();
- }
+ return skipSubTree();
+ } // If NullTo(Member)Pointer cast.
return true;
}
private:
- void ResetFirstSubExpr() { FirstSubExpr = 0; }
+ bool skipSubTree() { PruneSubtree = true; return true; }
private:
tooling::Replacements &Replace;
SourceManager &SM;
- const LangOptions &LangOpts;
+ ASTContext &Context;
const UserMacroNames &UserNullMacros;
unsigned &AcceptedChanges;
Expr *FirstSubExpr;
+ bool PruneSubtree;
};
+} // namespace
NullptrFixer::NullptrFixer(clang::tooling::Replacements &Replace,
unsigned &AcceptedChanges, RiskLevel)
@@ -169,14 +431,12 @@ NullptrFixer::NullptrFixer(clang::toolin
}
void NullptrFixer::run(const ast_matchers::MatchFinder::MatchResult &Result) {
- SourceManager &SM = *Result.SourceManager;
-
const CastExpr *NullCast = Result.Nodes.getNodeAs<CastExpr>(CastSequence);
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);
+ CastSequenceVisitor Visitor(Replace, *Result.Context, UserNullMacros,
+ AcceptedChanges);
Visitor.TraverseStmt(const_cast<CastExpr *>(NullCast));
}
Modified: clang-tools-extra/trunk/test/cpp11-migrate/UseNullptr/macros.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/cpp11-migrate/UseNullptr/macros.cpp?rev=181221&r1=181220&r2=181221&view=diff
==============================================================================
--- clang-tools-extra/trunk/test/cpp11-migrate/UseNullptr/macros.cpp (original)
+++ clang-tools-extra/trunk/test/cpp11-migrate/UseNullptr/macros.cpp Mon May 6 12:27:02 2013
@@ -79,29 +79,78 @@ void test_macro_expansion4() {
#undef MY_NULL
}
-// Test function-like macros where the parameter to the macro (expression)
-// results in a nullptr.
-void __dummy_assert_fail() {}
-
-void test_function_like_macro1() {
- // This tests that the CastSequenceVisitor is working properly.
-#define my_assert(expr) \
- ((expr) ? static_cast<void>(expr) : __dummy_assert_fail())
- // CHECK: ((expr) ? static_cast<void>(expr) : __dummy_assert_fail())
-
- int *p;
- my_assert(p != 0);
- // CHECK: my_assert(p != nullptr);
-#undef my_assert
-}
+#define IS_EQ(x, y) if (x != y) return;
+void test_macro_args() {
+ int i = 0;
+ int *Ptr;
+
+ IS_EQ(static_cast<int*>(0), Ptr);
+ // CHECK: IS_EQ(static_cast<int*>(nullptr), Ptr);
+ IS_EQ(0, Ptr); // literal
+ // CHECK: IS_EQ(nullptr, Ptr);
+ IS_EQ(NULL, Ptr); // macro
+ // CHECK: IS_EQ(nullptr, Ptr);
+
+ // These are ok since the null literal is not spelled within a macro.
+#define myassert(x) if (!(x)) return;
+ myassert(0 == Ptr);
+ // CHECK: myassert(nullptr == Ptr);
+ myassert(NULL == Ptr);
+ // CHECK: myassert(nullptr == Ptr);
+
+ // These are bad as the null literal is buried in a macro.
+#define BLAH(X) myassert(0 == (X));
+ // CHECK: #define BLAH(X) myassert(0 == (X));
+#define BLAH2(X) myassert(NULL == (X));
+ // CHECK: #define BLAH2(X) myassert(NULL == (X));
+ BLAH(Ptr);
+ // CHECK: BLAH(Ptr);
+ BLAH2(Ptr);
+ // CHECK: BLAH2(Ptr);
+
+ // Same as above but testing extra macro expansion.
+#define EXPECT_NULL(X) IS_EQ(0, X);
+ // CHECK: #define EXPECT_NULL(X) IS_EQ(0, X);
+#define EXPECT_NULL2(X) IS_EQ(NULL, X);
+ // CHECK: #define EXPECT_NULL2(X) IS_EQ(NULL, X);
+ EXPECT_NULL(Ptr);
+ // CHECK: EXPECT_NULL(Ptr);
+ EXPECT_NULL2(Ptr);
+ // CHECK: EXPECT_NULL2(Ptr);
+
+ // Almost the same as above but now null literal is not in a macro so ok
+ // to transform.
+#define EQUALS_PTR(X) IS_EQ(X, Ptr);
+ EQUALS_PTR(0);
+ EQUALS_PTR(NULL);
+
+ // Same as above but testing extra macro expansion.
+#define EQUALS_PTR_I(X) EQUALS_PTR(X)
+ EQUALS_PTR_I(0);
+ // CHECK: EQUALS_PTR_I(nullptr);
+ EQUALS_PTR_I(NULL);
+ // CHECK: EQUALS_PTR_I(nullptr);
+
+ // Ok since null literal not within macro. However, now testing macro
+ // used as arg to another macro.
+#define decorate(EXPR) side_effect(); EXPR;
+ decorate(IS_EQ(NULL, Ptr));
+ // CHECK: decorate(IS_EQ(nullptr, Ptr));
+ decorate(IS_EQ(0, Ptr));
+ // CHECK: decorate(IS_EQ(nullptr, Ptr));
+
+ // This macro causes a NullToPointer cast to happen where 0 is assigned to z
+ // but the 0 literal cannot be replaced because it is also used as an
+ // integer in the comparison.
+#define INT_AND_PTR_USE(X) do { int *z = X; if (X == 4) break; } while(false)
+ INT_AND_PTR_USE(0);
+ // CHECK: INT_AND_PTR_USE(0);
-void test_function_like_macro2() {
- // This tests that the implicit cast is working properly.
-#define my_macro(expr) \
- (expr)
-
- int *p;
- my_macro(p != 0);
- // CHECK: my_macro(p != nullptr);
-#undef my_macro
+ // Both uses of X in this case result in NullToPointer casts so replacement
+ // is possible.
+#define PTR_AND_PTR_USE(X) do { int *z = X; if (X != z) break; } while(false)
+ PTR_AND_PTR_USE(0);
+ // CHECK: PTR_AND_PTR_USE(nullptr);
+ PTR_AND_PTR_USE(NULL);
+ // CHECK: PTR_AND_PTR_USE(nullptr);
}
More information about the cfe-commits
mailing list