llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Eric Li (tJener) <details> <summary>Changes</summary> `DynTypedNode` has stored `ObjCProtocolLoc` by value since the node was added in 805f7a4fa4ce, but `NKI_ObjCProtocolLoc` was placed after `NKI_LastKindWithoutPointerIdentity`, so `hasPointerIdentity()` returned true for it. `getMemoizationData()` then returned (by chance?) the stored `ObjCProtocolDecl` pointer. As a result, two references to the same protocol at different locations compared equal through `DynTypedNode`. This caused `ASTContext::getParents()` to behave incorrectly. This commit changes `ObjCProtocolLoc` to be considered as not having pointer identity. Subsequently, we handle `ObjCProtocolLoc` in `DynTypedNode::operator<`, `operator==` and `getHashValue`, using both the protocol pointer and the source location. To catch this class of mismatch at compile time, this commit also introduces a `static_assert` to check that how it stores the node matches `hasPointerIdentity()` for the node kind. --- Full diff: https://github.com/llvm/llvm-project/pull/237451.diff 4 Files Affected: - (modified) clang/include/clang/AST/ASTTypeTraits.h (+41-4) - (modified) clang/lib/AST/ASTTypeTraits.cpp (+1-1) - (modified) clang/unittests/AST/ASTContextParentMapTest.cpp (+14) - (modified) clang/unittests/AST/ASTTypeTraitsTest.cpp (+39) ``````````diff diff --git a/clang/include/clang/AST/ASTTypeTraits.h b/clang/include/clang/AST/ASTTypeTraits.h index ee81826bfe1383..db2c2d451df937 100644 --- a/clang/include/clang/AST/ASTTypeTraits.h +++ b/clang/include/clang/AST/ASTTypeTraits.h @@ -133,6 +133,7 @@ class ASTNodeKind { NKI_TemplateName, NKI_NestedNameSpecifier, NKI_NestedNameSpecifierLoc, + NKI_ObjCProtocolLoc, NKI_QualType, #define TYPELOC(CLASS, PARENT) NKI_##CLASS##TypeLoc, #include "clang/AST/TypeLocNodes.def" @@ -156,7 +157,6 @@ class ASTNodeKind { NKI_Attr, #define ATTR(A) NKI_##A##Attr, #include "clang/Basic/AttrList.inc" - NKI_ObjCProtocolLoc, NKI_ConceptReference, NKI_OffsetOfNode, NKI_NumberOfKinds @@ -253,9 +253,15 @@ inline raw_ostream &operator<<(raw_ostream &OS, ASTNodeKind K) { class DynTypedNode { public: /// Creates a \c DynTypedNode from \c Node. - template <typename T> - static DynTypedNode create(const T &Node) { - return BaseConverter<T>::create(Node); + template <typename T> static DynTypedNode create(const T &Node) { + using Converter = BaseConverter<T>; + static_assert( + Converter::StoresPointer == + ASTNodeKind::getFromNodeKind<typename Converter::StoredType>() + .hasPointerIdentity(), + "hasPointerIdentity() must agree with whether DynTypedNode " + "stores a node by pointer"); + return Converter::create(Node); } /// Retrieve the stored node as type \c T. @@ -341,6 +347,15 @@ class DynTypedNode { return getUnchecked<NestedNameSpecifier>().getAsVoidPointer() < Other.getUnchecked<NestedNameSpecifier>().getAsVoidPointer(); + if (ASTNodeKind::getFromNodeKind<ObjCProtocolLoc>().isSame(NodeKind)) { + auto PLA = getUnchecked<ObjCProtocolLoc>(); + auto PLB = Other.getUnchecked<ObjCProtocolLoc>(); + return std::make_pair(PLA.getProtocol(), + PLA.getLocation().getRawEncoding()) < + std::make_pair(PLB.getProtocol(), + PLB.getLocation().getRawEncoding()); + } + assert(getMemoizationData() && Other.getMemoizationData()); return getMemoizationData() < Other.getMemoizationData(); } @@ -365,6 +380,13 @@ class DynTypedNode { return getUnchecked<NestedNameSpecifier>() == Other.getUnchecked<NestedNameSpecifier>(); + if (ASTNodeKind::getFromNodeKind<ObjCProtocolLoc>().isSame(NodeKind)) { + auto PLA = getUnchecked<ObjCProtocolLoc>(); + auto PLB = Other.getUnchecked<ObjCProtocolLoc>(); + return PLA.getProtocol() == PLB.getProtocol() && + PLA.getLocation() == PLB.getLocation(); + } + assert(getMemoizationData() && Other.getMemoizationData()); return getMemoizationData() == Other.getMemoizationData(); } @@ -396,6 +418,13 @@ class DynTypedNode { return llvm::DenseMapInfo<NestedNameSpecifier>::getHashValue( Val.getUnchecked<NestedNameSpecifier>()); + if (ASTNodeKind::getFromNodeKind<ObjCProtocolLoc>().isSame( + Val.NodeKind)) { + auto PL = Val.getUnchecked<ObjCProtocolLoc>(); + return llvm::hash_combine(PL.getProtocol(), + PL.getLocation().getRawEncoding()); + } + assert(Val.getMemoizationData()); return llvm::hash_value(Val.getMemoizationData()); } @@ -410,6 +439,8 @@ class DynTypedNode { /// Converter that uses dyn_cast<T> from a stored BaseT*. template <typename T, typename BaseT> struct DynCastPtrConverter { + static constexpr bool StoresPointer = true; + using StoredType = BaseT; static const T *get(ASTNodeKind NodeKind, const void *Storage) { if (ASTNodeKind::getFromNodeKind<T>().isBaseOf(NodeKind)) return &getUnchecked(NodeKind, Storage); @@ -430,6 +461,8 @@ class DynTypedNode { /// Converter that stores T* (by pointer). template <typename T> struct PtrConverter { + static constexpr bool StoresPointer = true; + using StoredType = T; static const T *get(ASTNodeKind NodeKind, const void *Storage) { if (ASTNodeKind::getFromNodeKind<T>().isSame(NodeKind)) return &getUnchecked(NodeKind, Storage); @@ -450,6 +483,8 @@ class DynTypedNode { /// Converter that stores T (by value). template <typename T> struct ValueConverter { + static constexpr bool StoresPointer = false; + using StoredType = T; static const T *get(ASTNodeKind NodeKind, const void *Storage) { if (ASTNodeKind::getFromNodeKind<T>().isSame(NodeKind)) return reinterpret_cast<const T *>(Storage); @@ -473,6 +508,8 @@ class DynTypedNode { template <typename T, typename BaseT, typename = std::enable_if_t<(sizeof(T) == sizeof(BaseT))>> struct DynCastValueConverter { + static constexpr bool StoresPointer = false; + using StoredType = BaseT; static const T *get(ASTNodeKind NodeKind, const void *Storage) { if (ASTNodeKind::getFromNodeKind<T>().isBaseOf(NodeKind)) return &getUnchecked(NodeKind, Storage); diff --git a/clang/lib/AST/ASTTypeTraits.cpp b/clang/lib/AST/ASTTypeTraits.cpp index 9e3b7786892b2c..05fb82380e6f02 100644 --- a/clang/lib/AST/ASTTypeTraits.cpp +++ b/clang/lib/AST/ASTTypeTraits.cpp @@ -32,6 +32,7 @@ const ASTNodeKind::KindInfo ASTNodeKind::AllKindInfo[] = { {NKI_None, "TemplateName"}, {NKI_None, "NestedNameSpecifier"}, {NKI_None, "NestedNameSpecifierLoc"}, + {NKI_None, "ObjCProtocolLoc"}, {NKI_None, "QualType"}, #define TYPELOC(CLASS, PARENT) {NKI_##PARENT, #CLASS "TypeLoc"}, #include "clang/AST/TypeLocNodes.def" @@ -54,7 +55,6 @@ const ASTNodeKind::KindInfo ASTNodeKind::AllKindInfo[] = { {NKI_None, "Attr"}, #define ATTR(A) {NKI_Attr, #A "Attr"}, #include "clang/Basic/AttrList.inc" - {NKI_None, "ObjCProtocolLoc"}, {NKI_None, "ConceptReference"}, {NKI_None, "OffsetOfNode"}, }; diff --git a/clang/unittests/AST/ASTContextParentMapTest.cpp b/clang/unittests/AST/ASTContextParentMapTest.cpp index 545eaf36191717..0e18c41e111901 100644 --- a/clang/unittests/AST/ASTContextParentMapTest.cpp +++ b/clang/unittests/AST/ASTContextParentMapTest.cpp @@ -49,6 +49,20 @@ TEST(GetParents, ReturnsParentForNestedNameSpecifierLoc) { nestedNameSpecifierLoc(hasParent(typeLoc())))); } +TEST(GetParents, ReturnsParentForObjCProtocolLoc) { + auto AST = tooling::buildASTFromCodeWithArgs( + "@protocol P @end @interface I <P> @end", {"-x", "objective-c"}); + ASTContext &Ctx = AST->getASTContext(); + auto Matches = match(objcInterfaceDecl(hasName("I")).bind("i"), Ctx); + ASSERT_EQ(Matches.size(), 1u); + const auto *I = Matches[0].getNodeAs<ObjCInterfaceDecl>("i"); + + ObjCProtocolLoc ProtocolLoc(*I->protocol_begin(), *I->protocol_loc_begin()); + auto Parents = Ctx.getParents(ProtocolLoc); + ASSERT_EQ(Parents.size(), 1u); + EXPECT_EQ(Parents[0].get<ObjCInterfaceDecl>(), I); +} + TEST(GetParents, ReturnsParentInsideTemplateInstantiations) { MatchVerifier<Decl> DeclVerifier; EXPECT_TRUE(DeclVerifier.match( diff --git a/clang/unittests/AST/ASTTypeTraitsTest.cpp b/clang/unittests/AST/ASTTypeTraitsTest.cpp index cb4adc9b8bf795..b444c4ebb7bb53 100644 --- a/clang/unittests/AST/ASTTypeTraitsTest.cpp +++ b/clang/unittests/AST/ASTTypeTraitsTest.cpp @@ -127,6 +127,7 @@ TEST(ASTNodeKind, ConstexprHasPointerIdentity) { EXPECT_FALSE(HasPointerIdentity<TypeLoc>); EXPECT_FALSE(HasPointerIdentity<QualType>); EXPECT_FALSE(HasPointerIdentity<NestedNameSpecifier>); + EXPECT_FALSE(HasPointerIdentity<ObjCProtocolLoc>); EXPECT_FALSE(HasPointerIdentity<Foo>); constexpr bool DefaultConstructedHasPointerIdentity = @@ -244,6 +245,44 @@ TEST(DynTypedNode, QualType) { EXPECT_FALSE(Node < Node); } +TEST(DynTypedNode, ObjCProtocolLoc) { + std::string code = R"cc( + @protocol P + @end + @interface I <P> + @end + @interface J <P> + @end + )cc"; + auto AST = + clang::tooling::buildASTFromCodeWithArgs(code, {"-x", "objective-c"}); + auto IMatches = + match(objcInterfaceDecl(hasName("I")).bind("i"), AST->getASTContext()); + auto JMatches = + match(objcInterfaceDecl(hasName("J")).bind("j"), AST->getASTContext()); + ASSERT_EQ(IMatches.size(), 1u); + ASSERT_EQ(JMatches.size(), 1u); + const auto *I = IMatches[0].getNodeAs<ObjCInterfaceDecl>("i"); + const auto *J = JMatches[0].getNodeAs<ObjCInterfaceDecl>("j"); + + // Both references name the same protocol at different locations. + ObjCProtocolLoc InI(*I->protocol_begin(), *I->protocol_loc_begin()); + ObjCProtocolLoc InJ(*J->protocol_begin(), *J->protocol_loc_begin()); + ASSERT_EQ(InI.getProtocol(), InJ.getProtocol()); + + DynTypedNode Node = DynTypedNode::create(InI); + DynTypedNode Copy = DynTypedNode::create(InI); + DynTypedNode Other = DynTypedNode::create(InJ); + EXPECT_EQ(Node.getMemoizationData(), nullptr); + EXPECT_TRUE(Node == Copy); + EXPECT_FALSE(Node < Copy); + EXPECT_FALSE(Copy < Node); + EXPECT_EQ(DynTypedNode::DenseMapInfo::getHashValue(Node), + DynTypedNode::DenseMapInfo::getHashValue(Copy)); + EXPECT_FALSE(Node == Other); + EXPECT_NE(Node < Other, Other < Node); +} + TEST(DynTypedNode, NestedNameSpecifier) { std::string code = R"cc(namespace a { struct b {}; } a::b x;)cc"; auto AST = clang::tooling::buildASTFromCode(code); `````````` </details> https://github.com/llvm/llvm-project/pull/237451 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
