Author: ioeric Date: Tue Sep 27 07:54:48 2016 New Revision: 282486 URL: http://llvm.org/viewvc/llvm-project?rev=282486&view=rev Log: Workaround ASTMatcher crashes. Added some more test cases.
Summary: - UsingDecl matcher crashed when `UsingShadowDecl` has no parent map. Workaround by moving parent check into `UsingDecl`. - FunctionDecl matcher crashed when there is a lambda defined in parameter list (also due to no parent map). Workaround by putting `unless(cxxMethodDecl())` before parent check. Reviewers: klimek, sbenza, aaron.ballman, hokein Subscribers: aaron.ballman, cfe-commits Differential Revision: https://reviews.llvm.org/D24862 Modified: clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp Modified: clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp?rev=282486&r1=282485&r2=282486&view=diff ============================================================================== --- clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp (original) +++ clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp Tue Sep 27 07:54:48 2016 @@ -256,9 +256,10 @@ void ChangeNamespaceTool::registerMatche auto DeclMatcher = namedDecl( hasAncestor(namespaceDecl()), unless(anyOf( - hasAncestor(namespaceDecl(isAnonymous())), + isImplicit(), hasAncestor(namespaceDecl(isAnonymous())), hasAncestor(cxxRecordDecl()), allOf(IsInMovedNs, unless(cxxRecordDecl(unless(isDefinition()))))))); + // Match TypeLocs on the declaration. Carefully match only the outermost // TypeLoc that's directly linked to the old class and don't handle nested // name specifier locs. @@ -271,10 +272,13 @@ void ChangeNamespaceTool::registerMatche hasAncestor(decl().bind("dc"))) .bind("type"), this); + // Types in `UsingShadowDecl` is not matched by `typeLoc` above, so we need to // special case it. Finder->addMatcher( - usingDecl(hasAnyUsingShadowDecl(IsInMovedNs)).bind("using_decl"), this); + usingDecl(IsInMovedNs, hasAnyUsingShadowDecl(decl())).bind("using_decl"), + this); + // Handle types in nested name specifier. Finder->addMatcher(nestedNameSpecifierLoc( hasAncestor(decl(IsInMovedNs).bind("dc")), @@ -284,18 +288,19 @@ void ChangeNamespaceTool::registerMatche this); // Handle function. - // Only handle functions that are defined in a namespace excluding static - // methods (qualified by nested specifier) and functions defined in the global - // namespace. + // Only handle functions that are defined in a namespace excluding member + // function, static methods (qualified by nested specifier), and functions + // defined in the global namespace. // Note that the matcher does not exclude calls to out-of-line static method // definitions, so we need to exclude them in the callback handler. - auto FuncMatcher = functionDecl( - hasParent(namespaceDecl()), - unless(anyOf(IsInMovedNs, hasAncestor(namespaceDecl(isAnonymous())), - hasAncestor(cxxRecordDecl())))); + auto FuncMatcher = + functionDecl(unless(anyOf(cxxMethodDecl(), IsInMovedNs, + hasAncestor(namespaceDecl(isAnonymous())), + hasAncestor(cxxRecordDecl()))), + hasParent(namespaceDecl())); Finder->addMatcher( decl(forEachDescendant(callExpr(callee(FuncMatcher)).bind("call")), - IsInMovedNs) + IsInMovedNs, unless(isImplicit())) .bind("dc"), this); } @@ -432,6 +437,8 @@ void ChangeNamespaceTool::moveClassForwa // Replaces a qualified symbol that refers to a declaration `DeclName` with the // shortest qualified name possible when the reference is in `NewNamespace`. +// FIXME: don't need to add redundant namespace qualifier when there is +// UsingShadowDecl or using namespace decl. void ChangeNamespaceTool::replaceQualifiedSymbolInDeclContext( const ast_matchers::MatchFinder::MatchResult &Result, const Decl *DeclCtx, SourceLocation Start, SourceLocation End, llvm::StringRef DeclName) { Modified: clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp?rev=282486&r1=282485&r2=282486&view=diff ============================================================================== --- clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp (original) +++ clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp Tue Sep 27 07:54:48 2016 @@ -229,8 +229,23 @@ TEST_F(ChangeNamespaceTest, MoveFunction EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code)); } +TEST_F(ChangeNamespaceTest, DoNotCrashWithLambdaAsParameter) { + std::string Code = + "#include <functional>\n" + "void f(std::function<void(int)> func, int param) { func(param); } " + "void g() { f([](int x) {}, 1); }"; + + std::string Expected = + "#include <functional>\n" + "void f(std::function<void(int)> func, int param) { func(param); } " + "void g() { f([](int x) {}, 1); }"; + EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code)); +} + TEST_F(ChangeNamespaceTest, FixUsingShadowDecl) { - std::string Code = "namespace na {\n" + std::string Code = "class GLOB {};\n" + "using BLOG = GLOB;\n" + "namespace na {\n" "namespace nc {\n" "class SAME {};\n" "}\n" @@ -245,7 +260,9 @@ TEST_F(ChangeNamespaceTest, FixUsingShad "} // namespace nb\n" "} // namespace na\n"; - std::string Expected = "namespace na {\n" + std::string Expected = "class GLOB {};\n" + "using BLOG = GLOB;\n" + "namespace na {\n" "namespace nc {\n" "class SAME {};\n" "}\n" @@ -263,6 +280,85 @@ TEST_F(ChangeNamespaceTest, FixUsingShad "} // namespace y\n" "} // namespace x\n"; EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code)); +} + +TEST_F(ChangeNamespaceTest, UsingShadowDeclInFunction) { + std::string Code = "namespace glob {\n" + "class Glob {};\n" + "}\n" + "namespace na {\n" + "namespace nb {\n" + "void f() {\n" + " using glob::Glob;\n" + " Glob g;\n" + "}\n" + "} // namespace nb\n" + "} // namespace na\n"; + + // FIXME: don't add namespace qualifier when there is UsingShadowDecl. + std::string Expected = "namespace glob {\n" + "class Glob {};\n" + "}\n" + "\n" + "namespace x {\n" + "namespace y {\n" + "void f() {\n" + " using ::glob::Glob;\n" + " glob::Glob g;\n" + "}\n" + "} // namespace y\n" + "} // namespace x\n"; + EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code)); +} + +TEST_F(ChangeNamespaceTest, UsingShadowDeclInGlobal) { + std::string Code = "namespace glob {\n" + "class Glob {};\n" + "}\n" + "using glob::Glob;\n" + "namespace na {\n" + "namespace nb {\n" + "void f() { Glob g; }\n" + "} // namespace nb\n" + "} // namespace na\n"; + + // FIXME: don't add namespace qualifier when there is UsingShadowDecl. + std::string Expected = "namespace glob {\n" + "class Glob {};\n" + "}\n" + "using glob::Glob;\n" + "\n" + "namespace x {\n" + "namespace y {\n" + "void f() { glob::Glob g; }\n" + "} // namespace y\n" + "} // namespace x\n"; + EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code)); +} + +TEST_F(ChangeNamespaceTest, UsingNamespace) { + std::string Code = "namespace glob {\n" + "class Glob {};\n" + "}\n" + "using namespace glob;\n" + "namespace na {\n" + "namespace nb {\n" + "void f() { Glob g; }\n" + "} // namespace nb\n" + "} // namespace na\n"; + + // FIXME: don't add namespace qualifier when there is "using namespace" decl. + std::string Expected = "namespace glob {\n" + "class Glob {};\n" + "}\n" + "using namespace glob;\n" + "\n" + "namespace x {\n" + "namespace y {\n" + "void f() { glob::Glob g; }\n" + "} // namespace y\n" + "} // namespace x\n"; + EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code)); } TEST_F(ChangeNamespaceTest, TypeInNestedNameSpecifier) { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
