[clang] Implement __is_virtual_base_of() intrinsic
#100393
Conversation
Endilll
added
clang:frontend
|
@llvm/pr-subscribers-clang Author: Vlad Serebrennikov (Endilll) ChangesThis patch adds compiler support for P2985R0 "A type trait for detecting virtual base classes". Resolves #98310. Full diff: https://github.com/llvm/llvm-project/pull/100393.diff 5 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index ac1de0db9ce48..242f50bf23924 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -80,6 +80,9 @@ C++23 Feature Support
C++2c Feature Support
^^^^^^^^^^^^^^^^^^^^^
+- Add ``__is_virtual_base_of`` intrinsic, which supports
+ `P2985R0 A type trait for detecting virtual base classes <https://wg21.link/p2985r0>`_
+
Resolutions to C++ Defect Reports
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Basic/TokenKinds.def b/clang/include/clang/Basic/TokenKinds.def
index 7f4912b9bcd96..1915e1f2588cb 100644
--- a/clang/include/clang/Basic/TokenKinds.def
+++ b/clang/include/clang/Basic/TokenKinds.def
@@ -529,6 +529,7 @@ TYPE_TRAIT_2(__is_trivially_assignable, IsTriviallyAssignable, KEYCXX)
TYPE_TRAIT_N(__is_trivially_constructible, IsTriviallyConstructible, KEYCXX)
TYPE_TRAIT_1(__is_trivially_copyable, IsTriviallyCopyable, KEYCXX)
TYPE_TRAIT_1(__is_union, IsUnion, KEYCXX)
+TYPE_TRAIT_2(__is_virtual_base_of, IsVirtualBaseOf, KEYCXX)
TYPE_TRAIT_1(__has_unique_object_representations,
HasUniqueObjectRepresentations, KEYCXX)
TYPE_TRAIT_2(__is_layout_compatible, IsLayoutCompatible, KEYCXX)
diff --git a/clang/lib/Parse/ParseExpr.cpp b/clang/lib/Parse/ParseExpr.cpp
index a12c375c8d48c..582db2075616c 100644
--- a/clang/lib/Parse/ParseExpr.cpp
+++ b/clang/lib/Parse/ParseExpr.cpp
@@ -822,6 +822,7 @@ bool Parser::isRevertibleTypeTrait(const IdentifierInfo *II,
REVERTIBLE_TYPE_TRAIT(__is_unbounded_array);
REVERTIBLE_TYPE_TRAIT(__is_union);
REVERTIBLE_TYPE_TRAIT(__is_unsigned);
+ REVERTIBLE_TYPE_TRAIT(__is_virtual_base_of);
REVERTIBLE_TYPE_TRAIT(__is_void);
REVERTIBLE_TYPE_TRAIT(__is_volatile);
REVERTIBLE_TYPE_TRAIT(__reference_binds_to_temporary);
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 14d1f395af90e..6cba3a62b694d 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -6027,6 +6027,33 @@ static bool EvaluateBinaryTypeTrait(Sema &Self, TypeTrait BTT, const TypeSourceI
return cast<CXXRecordDecl>(rhsRecord->getDecl())
->isDerivedFrom(cast<CXXRecordDecl>(lhsRecord->getDecl()));
}
+ case BTT_IsVirtualBaseOf: {
+ const RecordType *BaseRecord = LhsT->getAs<RecordType>();
+ const RecordType *DerivedRecord = RhsT->getAs<RecordType>();
+
+ if (!BaseRecord || !DerivedRecord) {
+ DiagnoseVLAInCXXTypeTrait(Self, Lhs, tok::kw___is_virtual_base_of);
+ DiagnoseVLAInCXXTypeTrait(Self, Rhs, tok::kw___is_virtual_base_of);
+ return false;
+ }
+
+ if (BaseRecord->isUnionType() || DerivedRecord->isUnionType())
+ return false;
+
+ if (!BaseRecord->isStructureOrClassType() ||
+ !DerivedRecord->isStructureOrClassType())
+ return false;
+
+ if (Self.RequireCompleteType(Rhs->getTypeLoc().getBeginLoc(), RhsT,
+ diag::err_incomplete_type))
+ return false;
+
+ if (Self.Context.hasSameUnqualifiedType(LhsT, RhsT))
+ return false;
+
+ return cast<CXXRecordDecl>(DerivedRecord->getDecl())
+ ->isVirtuallyDerivedFrom(cast<CXXRecordDecl>(BaseRecord->getDecl()));
+ }
case BTT_IsSame:
return Self.Context.hasSameType(LhsT, RhsT);
case BTT_TypeCompatible: {
diff --git a/clang/test/SemaCXX/type-traits.cpp b/clang/test/SemaCXX/type-traits.cpp
index 23b07cac13eaf..4eedcebfaeffd 100644
--- a/clang/test/SemaCXX/type-traits.cpp
+++ b/clang/test/SemaCXX/type-traits.cpp
@@ -2402,11 +2402,11 @@ template<typename T> struct DerivedB : BaseA<T> { };
template<typename T> struct CrazyDerived : T { };
-class class_forward; // expected-note 2 {{forward declaration of 'class_forward'}}
+class class_forward; // expected-note 4 {{forward declaration of 'class_forward'}}
template <class T> class DerivedTemp : Base {};
template <class T> class NonderivedTemp {};
-template <class T> class UndefinedTemp; // expected-note {{declared here}}
+template <class T> class UndefinedTemp; // expected-note 2 {{declared here}}
void is_base_of() {
static_assert(__is_base_of(Base, Derived));
@@ -2457,6 +2457,75 @@ void is_base_of() {
static_assert(!__is_base_of(DerivedB<int>, BaseA<int>));
}
+struct DerivedTransitiveViaNonVirtual : Derived3 {};
+struct DerivedTransitiveViaVirtual : virtual Derived3 {};
+
+template <typename T>
+struct CrazyDerivedVirtual : virtual T {};
+
+struct DerivedPrivate : private virtual Base {};
+struct DerivedProtected : protected virtual Base {};
+struct DerivedPrivatePrivate : private DerivedPrivate {};
+struct DerivedPrivateProtected : private DerivedProtected {};
+struct DerivedProtectedPrivate : protected DerivedProtected {};
+struct DerivedProtectedProtected : protected DerivedProtected {};
+
+void is_virtual_base_of(int n) {
+ static_assert(!__is_virtual_base_of(Base, Derived));
+ static_assert(!__is_virtual_base_of(const Base, Derived));
+ static_assert(!__is_virtual_base_of(Derived, Base));
+ static_assert(!__is_virtual_base_of(Derived, int));
+ static_assert(!__is_virtual_base_of(Base, Base));
+ static_assert(!__is_virtual_base_of(Base, Derived3));
+ static_assert(!__is_virtual_base_of(Derived, Derived3));
+ static_assert(__is_virtual_base_of(Derived2b, Derived3));
+ static_assert(__is_virtual_base_of(Derived2a, Derived3));
+ static_assert(!__is_virtual_base_of(BaseA<int>, DerivedB<int>));
+ static_assert(!__is_virtual_base_of(DerivedB<int>, BaseA<int>));
+ static_assert(!__is_virtual_base_of(Union, Union));
+ static_assert(!__is_virtual_base_of(Empty, Empty));
+ static_assert(!__is_virtual_base_of(class_forward, class_forward)); // expected-error {{incomplete type 'class_forward' where a complete type is required}}
+ static_assert(!__is_virtual_base_of(Empty, class_forward)); // expected-error {{incomplete type 'class_forward' where a complete type is required}}
+ static_assert(!__is_virtual_base_of(Base&, Derived&));
+ static_assert(!__is_virtual_base_of(Base[10], Derived[10]));
+ static_assert(!__is_virtual_base_of(Base[n], Derived[n])); // expected-error 2 {{variable length arrays are not supported in '__is_virtual_base_of'}}
+ static_assert(!__is_virtual_base_of(int, int));
+ static_assert(!__is_virtual_base_of(int[], int[]));
+ static_assert(!__is_virtual_base_of(long, int));
+ static_assert(!__is_virtual_base_of(Base, DerivedTemp<int>));
+ static_assert(!__is_virtual_base_of(Base, NonderivedTemp<int>));
+ static_assert(!__is_virtual_base_of(Base, UndefinedTemp<int>)); // expected-error {{implicit instantiation of undefined template 'UndefinedTemp<int>'}}
+ static_assert(__is_virtual_base_of(Base, DerivedPrivate));
+ static_assert(__is_virtual_base_of(Base, DerivedProtected));
+ static_assert(__is_virtual_base_of(Base, DerivedPrivatePrivate));
+ static_assert(__is_virtual_base_of(Base, DerivedPrivateProtected));
+ static_assert(__is_virtual_base_of(Base, DerivedProtectedPrivate));
+ static_assert(__is_virtual_base_of(Base, DerivedProtectedProtected));
+ static_assert(__is_virtual_base_of(Derived2a, DerivedTransitiveViaNonVirtual));
+ static_assert(__is_virtual_base_of(Derived2b, DerivedTransitiveViaNonVirtual));
+ static_assert(__is_virtual_base_of(Derived2a, DerivedTransitiveViaVirtual));
+ static_assert(__is_virtual_base_of(Derived2b, DerivedTransitiveViaVirtual));
+ static_assert(!__is_virtual_base_of(Base, CrazyDerived<Base>));
+ static_assert(!__is_virtual_base_of(CrazyDerived<Base>, Base));
+ static_assert(__is_virtual_base_of(Base, CrazyDerivedVirtual<Base>));
+ static_assert(!__is_virtual_base_of(CrazyDerivedVirtual<Base>, Base));
+
+ static_assert(!__is_virtual_base_of(IncompleteUnion, IncompleteUnion));
+ static_assert(!__is_virtual_base_of(Union, IncompleteUnion));
+ static_assert(!__is_virtual_base_of(IncompleteUnion, Union));
+ static_assert(!__is_virtual_base_of(IncompleteStruct, IncompleteUnion));
+ static_assert(!__is_virtual_base_of(IncompleteUnion, IncompleteStruct));
+ static_assert(!__is_virtual_base_of(Empty, IncompleteUnion));
+ static_assert(!__is_virtual_base_of(IncompleteUnion, Empty));
+ static_assert(!__is_virtual_base_of(int, IncompleteUnion));
+ static_assert(!__is_virtual_base_of(IncompleteUnion, int));
+ static_assert(!__is_virtual_base_of(Empty, Union));
+ static_assert(!__is_virtual_base_of(Union, Empty));
+ static_assert(!__is_virtual_base_of(int, Empty));
+ static_assert(!__is_virtual_base_of(Union, int));
+ static_assert(!__is_virtual_base_of(IncompleteStruct, IncompleteStruct[n])); // expected-error {{variable length arrays are not supported in '__is_virtual_base_of'}}
+}
+
template<class T, class U>
class TemplateClass {};
|
Endilll
changed the title
__is_virtual_base_of() intrinsic__is_virtual_base_of() intrinsic
| if (!BaseRecord->isStructureOrClassType() || | ||
| !DerivedRecord->isStructureOrClassType()) | ||
| return false; |
There was a problem hiding this comment.
Is this check necessary? If it’s not a union, it has to be a class type, right? Or am I missing something obvious?
There was a problem hiding this comment.
It could be an enum (enums can't have an inheritance relationship) - but this check is enough, we don't need the union check above.
I think this is a mismatch between the standard - which treats union as classes, and clang - which treats everything as record.
There was a problem hiding this comment.
It could be an enum
I thought a TagType was either a RecordType (struct/class/union) or an EnumType (enum). Can a RecordType really be an enum?
There was a problem hiding this comment.
IIRC ObjC interfaces are also RecordTypes, and I don't think we want to bother with them.
There was a problem hiding this comment.
ObjC types don't inherit from RecordType, so I don't think this check is needed.
There was a problem hiding this comment.
(and I don't think you can have a virtual objective c interface so there is nothing of ObjC to support here)
There was a problem hiding this comment.
isStructureOrClassType is defined as follows:
llvm-project/clang/lib/AST/Type.cpp
Lines 657 to 663 in 0ee32c4
So (1) ObjC doesn't have to inherit from RecordType to get involved, and (2) the function has a misleading name.
There was a problem hiding this comment.
Hmm, isInterface() is defined on TagDecl, so it seems that a RecordType can still not be an ObjcInterface from what I can tell. Looks like RD->isInterface() check in here is just pointless.
There was a problem hiding this comment.
It’s also maybe worth noting that the line containing the check is from 2014, so things may have been different back then
clang/lib/Sema/SemaExprCXX.cpp
Outdated
| if (Self.Context.hasSameUnqualifiedType(LhsT, RhsT)) | ||
| return false; |
There was a problem hiding this comment.
I’m assuming this is just short-circuiting? I’m aware that this case is mentioned in the paper (‘A class is never a virtual base class of itself’), but I don’t think it’s possible for a type to be its own base class—virtual or not—so I don’t think this is required for correctness. Is the check for virtual bases expensive?
There was a problem hiding this comment.
but I don’t think it’s possible for a type to be its own base class—virtual or not—
Well, std::is_base_of_v<T, T> is ought to be true per http://eel.is/c++draft/meta.rel#lib:is_base_of, even though I don't see this in core language wording (which might be the reason this is specified in the library in the first place).
Is the check for virtual bases expensive?
I haven't measured, but why do more work when we can help it?
There was a problem hiding this comment.
I see how it is, yeah, in that sense, it’s be easier to just keep it.
There was a problem hiding this comment.
I think this actually does more work instead of less; most code will not pass in the same type as both arguments, right? So that means this check won't catch anything useful most of the time, but the below check still will catch this case (I believe).
|
Can you update LanguageExtensions.rst? |
Done. |
| if (!BaseRecord->isStructureOrClassType() || | ||
| !DerivedRecord->isStructureOrClassType()) | ||
| return false; |
There was a problem hiding this comment.
ObjC types don't inherit from RecordType, so I don't think this check is needed.
clang/lib/Sema/SemaExprCXX.cpp
Outdated
| DiagnoseVLAInCXXTypeTrait(Self, Lhs, tok::kw___is_virtual_base_of); | ||
| DiagnoseVLAInCXXTypeTrait(Self, Rhs, tok::kw___is_virtual_base_of); |
There was a problem hiding this comment.
I guess I'm surprised by this -- why are VLAs special?
There was a problem hiding this comment.
We’ve been generally disallowing VLA’s in type-traits recently; there was some discussion about this somewhere. Don’t remember where exactly, but I think #88646 is a good starting point.
There was a problem hiding this comment.
I don't want to introduce even more VLA extensions in C++ mode (this builtin is exposed only in C++ mode). This began with #88473 (comment)
| !DerivedRecord->isStructureOrClassType()) | ||
| return false; | ||
|
|
||
| if (Self.RequireCompleteType(Rhs->getTypeLoc().getBeginLoc(), RhsT, |
There was a problem hiding this comment.
What does it mean for there to be a complete derived class type but an incomplete base class type? Shouldn't this require a complete type in both cases to give reasonable diagnostic behavior?
There was a problem hiding this comment.
The proposal only mentions that the second type has to be complete. My guess is that the reasoning behind this is that in e.g. __is_virtual_base_of(A, B), if B is complete but A isn’t, then A can’t be a base of B—since a complete class can’t have an incomplete base—and therefore, it can’t be a virtual base either.
There was a problem hiding this comment.
I hadn't realized that is_base_of already has the same requirement, so I don't insist on a change here. But that said, this presumes users don't have typos they'd like to catch. e.g.,
struct Bee { };
struct Be;
struct Derived : virtual Bee {};
static_assert(!__is_virtual_base_of(Be, Derived)); // Oops, I can't spell very well!
I think it would be much kinder to tell the user that Be is incomplete than let them be surprised.
There was a problem hiding this comment.
What does it mean for there to be a complete derived class type but an incomplete base class type? Shouldn't this require a complete type in both cases to give reasonable diagnostic behavior?
There is no incomplete base type. We are asking whether some type is a base, and if it isn't, we don't care that it is complete. Type traits don't mandate completeness if they don't need to.
clang/lib/Sema/SemaExprCXX.cpp
Outdated
| if (Self.Context.hasSameUnqualifiedType(LhsT, RhsT)) | ||
| return false; |
There was a problem hiding this comment.
I think this actually does more work instead of less; most code will not pass in the same type as both arguments, right? So that means this check won't catch anything useful most of the time, but the below check still will catch this case (I believe).
|
Following discussion in #98310, we should rename |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@PeterChou1 I see you had landed |
|
The CI issues are unrelated so I'll merge this (someone should investigate why @Endilll Thanks for the patch :) |
|
The libc++ part: #105847 |
This patch adds compiler support for P2985R0 "A type trait for detecting virtual base classes".
Like we recently did with
__is_layout_compatible()and__is_pointer_interconvertible_base_of(), we support it only in C++ mode, and reject VLAs.Resolves #98310.