[clang-tools-extra] r249291 - Fix bug in modernize-use-nullptr.
Angel Garcia Gomez via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 5 01:40:14 PDT 2015
Author: angelgarcia
Date: Mon Oct 5 03:40:13 2015
New Revision: 249291
URL: http://llvm.org/viewvc/llvm-project?rev=249291&view=rev
Log:
Fix bug in modernize-use-nullptr.
Summary:
https://llvm.org/bugs/show_bug.cgi?id=24960
modernize-use-nullptr would hit an assertion in some cases involving macros and initializer lists, due to finding a node with more than one parent (the two forms of the initializer list).
However, this doesn't mean that the replacement is incorrect, so instead of just rejecting this case I tried to find a way to make it work. Looking at the semantic form of the InitListExpr made sense to me (looking at both forms results in false negatives) but I am not sure of the things that we can miss by skipping the syntactic form.
Reviewers: klimek
Subscribers: cfe-commits, alexfh
Differential Revision: http://reviews.llvm.org/D13246
Modified:
clang-tools-extra/trunk/clang-tidy/modernize/UseNullptrCheck.cpp
clang-tools-extra/trunk/test/clang-tidy/modernize-use-nullptr.cpp
Modified: clang-tools-extra/trunk/clang-tidy/modernize/UseNullptrCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/modernize/UseNullptrCheck.cpp?rev=249291&r1=249290&r2=249291&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/modernize/UseNullptrCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/modernize/UseNullptrCheck.cpp Mon Oct 5 03:40:13 2015
@@ -150,6 +150,16 @@ public:
return true;
}
+ bool TraverseInitListExpr(InitListExpr *S) {
+ // Only go through the semantic form of the InitListExpr, because
+ // ImplicitCast might not appear in the syntactic form, and this results in
+ // finding usages of the macro argument that don't have a ImplicitCast as an
+ // ancestor (thus invalidating the replacement) when they actually have.
+ return RecursiveASTVisitor<MacroArgUsageVisitor>::
+ TraverseSynOrSemInitListExpr(
+ S->isSemanticForm() ? S : S->getSemanticForm());
+ }
+
bool foundInvalid() const { return InvalidFound; }
private:
@@ -273,7 +283,7 @@ private:
// 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
+ // Visitor needs: arg loc.
MacroArgUsageVisitor ArgUsageVisitor(SM.getFileLoc(CastLoc), SM);
if (const auto *D = ContainingAncestor.get<Decl>())
ArgUsageVisitor.TraverseDecl(const_cast<Decl *>(D));
@@ -345,8 +355,8 @@ private:
/// also handled.
///
/// False means either:
- /// - TestLoc is not from a macro expansion
- /// - TestLoc is from a different macro expansion
+ /// - TestLoc is not from a macro expansion.
+ /// - TestLoc is from a different macro expansion.
bool expandsFrom(SourceLocation TestLoc, SourceLocation TestMacroLoc) {
if (TestLoc.isFileID()) {
return false;
@@ -399,8 +409,7 @@ private:
ast_type_traits::DynTypedNode &Result) {
// 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.
+ // parent, except for the case specified below.
assert(MacroLoc.isFileID());
@@ -408,8 +417,16 @@ private:
const auto &Parents = Context.getParents(Start);
if (Parents.empty())
return false;
- assert(Parents.size() == 1 &&
- "Found an ancestor with more than one parent!");
+ if (Parents.size() > 1) {
+ // If there are more than one parents, don't do the replacement unless
+ // they are InitListsExpr (semantic and syntactic form). In this case we
+ // can choose any one here, and the ASTVisitor will take care of
+ // traversing the right one.
+ for (const auto &Parent : Parents) {
+ if (!Parent.get<InitListExpr>())
+ return false;
+ }
+ }
const ast_type_traits::DynTypedNode &Parent = Parents[0];
Modified: clang-tools-extra/trunk/test/clang-tidy/modernize-use-nullptr.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/modernize-use-nullptr.cpp?rev=249291&r1=249290&r2=249291&view=diff
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/modernize-use-nullptr.cpp (original)
+++ clang-tools-extra/trunk/test/clang-tidy/modernize-use-nullptr.cpp Mon Oct 5 03:40:13 2015
@@ -174,4 +174,13 @@ void test_macro_args() {
#define CALL(X) X
OPTIONAL_CODE(NOT_NULL);
CALL(NOT_NULL);
+
+#define ENTRY(X) {X}
+ struct A {
+ int *Ptr;
+ } a[2] = {ENTRY(0), {0}};
+ // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use nullptr
+ // CHECK-MESSAGES: :[[@LINE-2]]:24: warning: use nullptr
+ // CHECK-FIXES: a[2] = {ENTRY(nullptr), {nullptr}};
+#undef ENTRY
}
More information about the cfe-commits
mailing list