Skip to content

[clang] Implement __is_virtual_base_of() intrinsic #100393

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jul 26, 2024
Merged

[clang] Implement __is_virtual_base_of() intrinsic #100393

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
772b5d8
[clang] Impement `__is_virtual_base_of()` intrinsic
Endilll Jul 24, 2024
8b0b0c5
Address reviewer feedback
Endilll Jul 24, 2024
da449c9
Remove short-circuiting
Endilll Jul 25, 2024
8913119
Rename to `__builtin_is_virtual_base_of`
Endilll Jul 25, 2024
46f02b2
Apply clang-format suggestions
Endilll Jul 25, 2024
60ccd79
Merge remote-tracking branch 'upstream/main' into is-virtual-base-of
Endilll Jul 25, 2024
8806fdb
Merge remote-tracking branch 'upstream/main' into is-virtual-base-of
Endilll Jul 25, 2024
47e274d
Merge remote-tracking branch 'upstream/main' into is-virtual-base-of
Endilll Jul 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions clang/docs/LanguageExtensions.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1660,6 +1660,7 @@ The following type trait primitives are supported by Clang. Those traits marked
* ``__is_unsigned`` (C++, Embarcadero):
Returns false for enumeration types. Note, before Clang 13, returned true for
enumeration types if the underlying type was unsigned.
* ``__is_virtual_base_of`` (C++, GNU, Microsoft)
* ``__is_void`` (C++, Embarcadero)
* ``__is_volatile`` (C++, Embarcadero)
* ``__reference_binds_to_temporary(T, U)`` (Clang): Determines whether a
Expand Down
3 changes: 3 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Expand Down
1 change: 1 addition & 0 deletions clang/include/clang/Basic/TokenKinds.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
27 changes: 27 additions & 0 deletions clang/lib/Sema/SemaExprCXX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'm surprised by this -- why are VLAs special?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

return false;
}

if (BaseRecord->isUnionType() || DerivedRecord->isUnionType())
return false;

if (!BaseRecord->isStructureOrClassType() ||
!DerivedRecord->isStructureOrClassType())
return false;
Comment on lines +6048 to +6050
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this check necessary? If it’s not a union, it has to be a class type, right? Or am I missing something obvious?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC ObjC interfaces are also RecordTypes, and I don't think we want to bother with them.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ObjC types don't inherit from RecordType, so I don't think this check is needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and I don't think you can have a virtual objective c interface so there is nothing of ObjC to support here)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isStructureOrClassType is defined as follows:

llvm-project/clang/lib/AST/Type.cpp

Lines 657 to 663 in 0ee32c4

bool Type::isStructureOrClassType() const {
if (const auto *RT = getAs<RecordType>()) {
RecordDecl *RD = RT->getDecl();
return RD->isStruct() || RD->isClass() || RD->isInterface();
}
return false;
}

So (1) ObjC doesn't have to inherit from RecordType to get involved, and (2) the function has a misleading name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s also maybe worth noting that the line containing the check is from 2014, so things may have been different back then


if (Self.RequireCompleteType(Rhs->getTypeLoc().getBeginLoc(), RhsT,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

diag::err_incomplete_type))
return false;

if (Self.Context.hasSameUnqualifiedType(LhsT, RhsT))
return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see how it is, yeah, in that sense, it’s be easier to just keep it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


return cast<CXXRecordDecl>(DerivedRecord->getDecl())
->isVirtuallyDerivedFrom(cast<CXXRecordDecl>(BaseRecord->getDecl()));
}
case BTT_IsSame:
return Self.Context.hasSameType(LhsT, RhsT);
case BTT_TypeCompatible: {
Expand Down
74 changes: 72 additions & 2 deletions clang/test/SemaCXX/type-traits.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down Expand Up @@ -2457,6 +2457,76 @@ 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(class_forward, Empty));
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 {};

Expand Down
Loading