From: Samuel Benzaquen Date: Mon, 6 Oct 2014 13:14:30 +0000 (+0000) Subject: Fix bug in DynTypedMatcher::constructVariadic() that would cause false negatives. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=ea9c99d214345b0c6f97f7293708b78b7744eaf3;p=clang Fix bug in DynTypedMatcher::constructVariadic() that would cause false negatives. Summary: DynTypedMatcher::constructVariadic() where the restrict kind of the different matchers are not related causes the matcher to have a "None" restrict kind. This causes false negatives for anyOf and eachOf. Change the logic to get a common ancestor if there is one. Also added regression tests that fail without the fix. Reviewers: klimek Subscribers: klimek, cfe-commits Differential Revision: http://reviews.llvm.org/D5580 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@219118 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/AST/ASTTypeTraits.h b/include/clang/AST/ASTTypeTraits.h index 01c5cec599..efeac563db 100644 --- a/include/clang/AST/ASTTypeTraits.h +++ b/include/clang/AST/ASTTypeTraits.h @@ -63,6 +63,9 @@ public: /// \brief Returns \c true if \c this and \c Other represent the same kind. bool isSame(ASTNodeKind Other) const; + /// \brief Returns \c true only for the default \c ASTNodeKind() + bool isNone() const { return KindId == NKI_None; } + /// \brief Returns \c true if \c this is a base kind of (or same as) \c Other. /// \param Distance If non-null, used to return the distance between \c this /// and \c Other in the class hierarchy. @@ -76,6 +79,17 @@ public: return KindId < Other.KindId; } + /// \brief Return the most derived type between \p Kind1 and \p Kind2. + /// + /// Return ASTNodeKind() if they are not related. + static ASTNodeKind getMostDerivedType(ASTNodeKind Kind1, ASTNodeKind Kind2); + + /// \brief Return the most derived common ancestor between Kind1 and Kind2. + /// + /// Return ASTNodeKind() if they are not related. + static ASTNodeKind getMostDerivedCommonAncestor(ASTNodeKind Kind1, + ASTNodeKind Kind2); + private: /// \brief Kind ids. /// diff --git a/lib/AST/ASTTypeTraits.cpp b/lib/AST/ASTTypeTraits.cpp index 56915b9640..ec0671ceb1 100644 --- a/lib/AST/ASTTypeTraits.cpp +++ b/lib/AST/ASTTypeTraits.cpp @@ -62,6 +62,22 @@ bool ASTNodeKind::isBaseOf(NodeKindId Base, NodeKindId Derived, StringRef ASTNodeKind::asStringRef() const { return AllKindInfo[KindId].Name; } +ASTNodeKind ASTNodeKind::getMostDerivedType(ASTNodeKind Kind1, + ASTNodeKind Kind2) { + if (Kind1.isBaseOf(Kind2)) return Kind2; + if (Kind2.isBaseOf(Kind1)) return Kind1; + return ASTNodeKind(); +} + +ASTNodeKind ASTNodeKind::getMostDerivedCommonAncestor(ASTNodeKind Kind1, + ASTNodeKind Kind2) { + NodeKindId Parent = Kind1.KindId; + while (!isBaseOf(Parent, Kind2.KindId, nullptr) && Parent != NKI_None) { + Parent = AllKindInfo[Parent].ParentId; + } + return ASTNodeKind(Parent); +} + ASTNodeKind ASTNodeKind::getFromNode(const Decl &D) { switch (D.getKind()) { #define DECL(DERIVED, BASE) \ diff --git a/lib/ASTMatchers/ASTMatchersInternal.cpp b/lib/ASTMatchers/ASTMatchersInternal.cpp index 4cff381f3f..ec60f0d949 100644 --- a/lib/ASTMatchers/ASTMatchersInternal.cpp +++ b/lib/ASTMatchers/ASTMatchersInternal.cpp @@ -64,28 +64,6 @@ class IdDynMatcher : public DynMatcherInterface { const IntrusiveRefCntPtr InnerMatcher; }; -/// \brief Return the most derived type between \p Kind1 and \p Kind2. -/// -/// Return the null type if they are not related. -ast_type_traits::ASTNodeKind getMostDerivedType( - const ast_type_traits::ASTNodeKind Kind1, - const ast_type_traits::ASTNodeKind Kind2) { - if (Kind1.isBaseOf(Kind2)) return Kind2; - if (Kind2.isBaseOf(Kind1)) return Kind1; - return ast_type_traits::ASTNodeKind(); -} - -/// \brief Return the least derived type between \p Kind1 and \p Kind2. -/// -/// Return the null type if they are not related. -static ast_type_traits::ASTNodeKind getLeastDerivedType( - const ast_type_traits::ASTNodeKind Kind1, - const ast_type_traits::ASTNodeKind Kind2) { - if (Kind1.isBaseOf(Kind2)) return Kind1; - if (Kind2.isBaseOf(Kind1)) return Kind2; - return ast_type_traits::ASTNodeKind(); -} - } // namespace DynTypedMatcher DynTypedMatcher::constructVariadic( @@ -98,7 +76,8 @@ DynTypedMatcher DynTypedMatcher::constructVariadic( assert(Result.SupportedKind.isSame(M.SupportedKind) && "SupportedKind must match!"); Result.RestrictKind = - getLeastDerivedType(Result.RestrictKind, M.RestrictKind); + ast_type_traits::ASTNodeKind::getMostDerivedCommonAncestor( + Result.RestrictKind, M.RestrictKind); } Result.Implementation = new VariadicMatcher(Func, std::move(InnerMatchers)); return Result; @@ -108,7 +87,8 @@ DynTypedMatcher DynTypedMatcher::dynCastTo( const ast_type_traits::ASTNodeKind Kind) const { auto Copy = *this; Copy.SupportedKind = Kind; - Copy.RestrictKind = getMostDerivedType(Kind, RestrictKind); + Copy.RestrictKind = + ast_type_traits::ASTNodeKind::getMostDerivedType(Kind, RestrictKind); return Copy; } diff --git a/unittests/AST/ASTTypeTraitsTest.cpp b/unittests/AST/ASTTypeTraitsTest.cpp index 0e73f76169..eeb01ccad1 100644 --- a/unittests/AST/ASTTypeTraitsTest.cpp +++ b/unittests/AST/ASTTypeTraitsTest.cpp @@ -26,6 +26,12 @@ template static ASTNodeKind DNT() { return ASTNodeKind::getFromNodeKind(); } +TEST(ASTNodeKind, IsNone) { + EXPECT_TRUE(ASTNodeKind().isNone()); + EXPECT_FALSE(DNT().isNone()); + EXPECT_FALSE(DNT().isNone()); +} + TEST(ASTNodeKind, Bases) { EXPECT_TRUE(DNT().isBaseOf(DNT())); EXPECT_FALSE(DNT().isSame(DNT())); @@ -60,6 +66,39 @@ TEST(ASTNodeKind, DiffBase) { EXPECT_FALSE(DNT().isSame(DNT())); } +TEST(ASTNodeKind, MostDerivedType) { + EXPECT_TRUE(DNT().isSame( + ASTNodeKind::getMostDerivedType(DNT(), DNT()))); + EXPECT_TRUE(DNT().isSame( + ASTNodeKind::getMostDerivedType(DNT(), DNT()))); + EXPECT_TRUE(DNT().isSame( + ASTNodeKind::getMostDerivedType(DNT(), DNT()))); + + // Not related. Returns nothing. + EXPECT_TRUE( + ASTNodeKind::getMostDerivedType(DNT(), DNT()).isNone()); + EXPECT_TRUE(ASTNodeKind::getMostDerivedType(DNT(), + DNT()).isNone()); +} + +TEST(ASTNodeKind, MostDerivedCommonAncestor) { + EXPECT_TRUE(DNT().isSame(ASTNodeKind::getMostDerivedCommonAncestor( + DNT(), DNT()))); + EXPECT_TRUE(DNT().isSame(ASTNodeKind::getMostDerivedCommonAncestor( + DNT(), DNT()))); + EXPECT_TRUE(DNT().isSame(ASTNodeKind::getMostDerivedCommonAncestor( + DNT(), DNT()))); + + // A little related. Returns the ancestor. + EXPECT_TRUE( + DNT().isSame(ASTNodeKind::getMostDerivedCommonAncestor( + DNT(), DNT()))); + + // Not related. Returns nothing. + EXPECT_TRUE(ASTNodeKind::getMostDerivedCommonAncestor( + DNT(), DNT()).isNone()); +} + struct Foo {}; TEST(ASTNodeKind, UnknownKind) { diff --git a/unittests/ASTMatchers/ASTMatchersTest.cpp b/unittests/ASTMatchers/ASTMatchersTest.cpp index 4eb4c0d672..a7969340ba 100644 --- a/unittests/ASTMatchers/ASTMatchersTest.cpp +++ b/unittests/ASTMatchers/ASTMatchersTest.cpp @@ -460,6 +460,11 @@ TEST(DeclarationMatcher, MatchAnyOf) { EXPECT_TRUE(matches("class U {};", XOrYOrZOrUOrV)); EXPECT_TRUE(matches("class V {};", XOrYOrZOrUOrV)); EXPECT_TRUE(notMatches("class A {};", XOrYOrZOrUOrV)); + + StatementMatcher MixedTypes = stmt(anyOf(ifStmt(), binaryOperator())); + EXPECT_TRUE(matches("int F() { return 1 + 2; }", MixedTypes)); + EXPECT_TRUE(matches("int F() { if (true) return 1; }", MixedTypes)); + EXPECT_TRUE(notMatches("int F() { return 1; }", MixedTypes)); } TEST(DeclarationMatcher, MatchHas) { diff --git a/unittests/ASTMatchers/Dynamic/RegistryTest.cpp b/unittests/ASTMatchers/Dynamic/RegistryTest.cpp index f8a9430f16..824b310442 100644 --- a/unittests/ASTMatchers/Dynamic/RegistryTest.cpp +++ b/unittests/ASTMatchers/Dynamic/RegistryTest.cpp @@ -347,7 +347,7 @@ TEST_F(RegistryTest, VariadicOp) { "anyOf", constructMatcher("recordDecl", constructMatcher("hasName", std::string("Foo"))), - constructMatcher("namedDecl", + constructMatcher("functionDecl", constructMatcher("hasName", std::string("foo")))) .getTypedMatcher();