Author: sbenza Date: Mon Oct 6 08:14:30 2014 New Revision: 219118 URL: http://llvm.org/viewvc/llvm-project?rev=219118&view=rev Log: 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 Modified: cfe/trunk/include/clang/AST/ASTTypeTraits.h cfe/trunk/lib/AST/ASTTypeTraits.cpp cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp cfe/trunk/unittests/AST/ASTTypeTraitsTest.cpp cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp cfe/trunk/unittests/ASTMatchers/Dynamic/RegistryTest.cpp Modified: cfe/trunk/include/clang/AST/ASTTypeTraits.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTTypeTraits.h?rev=219118&r1=219117&r2=219118&view=diff ============================================================================== --- cfe/trunk/include/clang/AST/ASTTypeTraits.h (original) +++ cfe/trunk/include/clang/AST/ASTTypeTraits.h Mon Oct 6 08:14:30 2014 @@ -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. /// Modified: cfe/trunk/lib/AST/ASTTypeTraits.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTTypeTraits.cpp?rev=219118&r1=219117&r2=219118&view=diff ============================================================================== --- cfe/trunk/lib/AST/ASTTypeTraits.cpp (original) +++ cfe/trunk/lib/AST/ASTTypeTraits.cpp Mon Oct 6 08:14:30 2014 @@ -62,6 +62,22 @@ bool ASTNodeKind::isBaseOf(NodeKindId Ba 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) \ Modified: cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp?rev=219118&r1=219117&r2=219118&view=diff ============================================================================== --- cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp (original) +++ cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp Mon Oct 6 08:14:30 2014 @@ -64,28 +64,6 @@ class IdDynMatcher : public DynMatcherIn const IntrusiveRefCntPtr<DynMatcherInterface> 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::constru 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::dynCast 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; } Modified: cfe/trunk/unittests/AST/ASTTypeTraitsTest.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/AST/ASTTypeTraitsTest.cpp?rev=219118&r1=219117&r2=219118&view=diff ============================================================================== --- cfe/trunk/unittests/AST/ASTTypeTraitsTest.cpp (original) +++ cfe/trunk/unittests/AST/ASTTypeTraitsTest.cpp Mon Oct 6 08:14:30 2014 @@ -26,6 +26,12 @@ template <typename T> static ASTNodeKind return ASTNodeKind::getFromNodeKind<T>(); } +TEST(ASTNodeKind, IsNone) { + EXPECT_TRUE(ASTNodeKind().isNone()); + EXPECT_FALSE(DNT<Decl>().isNone()); + EXPECT_FALSE(DNT<VarDecl>().isNone()); +} + TEST(ASTNodeKind, Bases) { EXPECT_TRUE(DNT<Decl>().isBaseOf(DNT<VarDecl>())); EXPECT_FALSE(DNT<Decl>().isSame(DNT<VarDecl>())); @@ -60,6 +66,39 @@ TEST(ASTNodeKind, DiffBase) { EXPECT_FALSE(DNT<Type>().isSame(DNT<QualType>())); } +TEST(ASTNodeKind, MostDerivedType) { + EXPECT_TRUE(DNT<BinaryOperator>().isSame( + ASTNodeKind::getMostDerivedType(DNT<Expr>(), DNT<BinaryOperator>()))); + EXPECT_TRUE(DNT<BinaryOperator>().isSame( + ASTNodeKind::getMostDerivedType(DNT<BinaryOperator>(), DNT<Expr>()))); + EXPECT_TRUE(DNT<VarDecl>().isSame( + ASTNodeKind::getMostDerivedType(DNT<VarDecl>(), DNT<VarDecl>()))); + + // Not related. Returns nothing. + EXPECT_TRUE( + ASTNodeKind::getMostDerivedType(DNT<IfStmt>(), DNT<VarDecl>()).isNone()); + EXPECT_TRUE(ASTNodeKind::getMostDerivedType(DNT<IfStmt>(), + DNT<BinaryOperator>()).isNone()); +} + +TEST(ASTNodeKind, MostDerivedCommonAncestor) { + EXPECT_TRUE(DNT<Expr>().isSame(ASTNodeKind::getMostDerivedCommonAncestor( + DNT<Expr>(), DNT<BinaryOperator>()))); + EXPECT_TRUE(DNT<Expr>().isSame(ASTNodeKind::getMostDerivedCommonAncestor( + DNT<BinaryOperator>(), DNT<Expr>()))); + EXPECT_TRUE(DNT<VarDecl>().isSame(ASTNodeKind::getMostDerivedCommonAncestor( + DNT<VarDecl>(), DNT<VarDecl>()))); + + // A little related. Returns the ancestor. + EXPECT_TRUE( + DNT<NamedDecl>().isSame(ASTNodeKind::getMostDerivedCommonAncestor( + DNT<CXXMethodDecl>(), DNT<RecordDecl>()))); + + // Not related. Returns nothing. + EXPECT_TRUE(ASTNodeKind::getMostDerivedCommonAncestor( + DNT<IfStmt>(), DNT<VarDecl>()).isNone()); +} + struct Foo {}; TEST(ASTNodeKind, UnknownKind) { Modified: cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp?rev=219118&r1=219117&r2=219118&view=diff ============================================================================== --- cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp (original) +++ cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp Mon Oct 6 08:14:30 2014 @@ -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) { Modified: cfe/trunk/unittests/ASTMatchers/Dynamic/RegistryTest.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/ASTMatchers/Dynamic/RegistryTest.cpp?rev=219118&r1=219117&r2=219118&view=diff ============================================================================== --- cfe/trunk/unittests/ASTMatchers/Dynamic/RegistryTest.cpp (original) +++ cfe/trunk/unittests/ASTMatchers/Dynamic/RegistryTest.cpp Mon Oct 6 08:14:30 2014 @@ -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<Decl>(); _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
