Skip to content
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

Jump to bottom

P2985R0 A type trait for detecting virtual base classes #98310

Closed
Endilll opened this issue Jul 10, 2024 · 16 comments · Fixed by #100393
Closed

P2985R0 A type trait for detecting virtual base classes #98310

Endilll opened this issue Jul 10, 2024 · 16 comments · Fixed by #100393
Assignees
@Endilll
Labels
c++26 clang:frontend Language frontend issues, e.g. anything involving "Sema"

Comments

@Endilll
Copy link
Contributor

Endilll commented Jul 10, 2024

This paper introduces std::is_virtual_base_of trait, and was voted into the working draft in St. Louis.
Like other traits, we're planning to support this with a __is_virtual_base_of builtin.
Tagging @stl, @jwakely, and @ldionne to make sure they are fine with this spelling, and hopefully to prevent implementation divergence on the name.
@Endilll Endilll added clang:frontend Language frontend issues, e.g. anything involving "Sema" c++26 labels Jul 10, 2024
@Endilll Endilll self-assigned this Jul 10, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 10, 2024

@llvm/issue-subscribers-clang-frontend

Author: Vlad Serebrennikov (Endilll)

This paper introduces `std::is_virtual_base_of` trait, and was voted into the working draft in St. Louis. Like other traits, we're planning to support this with a `__is_virtual_base_of` builtin. Tagging @STL, @jwakely, and @ldionne to make sure they are fine with this spelling, and hopefully to prevent implementation divergence on the name.

@Endilll
Copy link
Contributor Author

Endilll commented Jul 10, 2024

@StephanTLavavej

@ldionne
Copy link
Member

ldionne commented Jul 10, 2024

The name LGTM. For what it's worth, I would have preferred if all of the type trait builtins started with __builtin (eg __builtin_is_signed(...)), but at this point we should just be consistent with everything else.

@StephanTLavavej
Copy link
Member

Name LGTM too.

@jwakely
Copy link
Contributor

jwakely commented Jul 17, 2024

For what it's worth, I would have preferred if all of the type trait builtins started with __builtin (eg __builtin_is_signed(...)), but at this point we should just be consistent with everything else.

Yes, we had a similar conversation in GCC recently. That would have avoided all the problems with new built-in traits clashing with existing class templates already using the name (e.g. __is_pointer, __is_assignable). But it seems far too late to change this now.

The suggested name seems coherent with existing practice.

@StephanTLavavej StephanTLavavej mentioned this issue Jul 17, 2024
Open
@Endilll Endilll mentioned this issue Jul 24, 2024
Merged
@ldionne
Copy link
Member

ldionne commented Jul 24, 2024 •
edited
Loading

@jwakely Would it be controversial to propose that we support both the __builtin_XXXXX spelling and the normal spelling in compilers for a while, and perhaps document that we want to move away from the non-__builtin_XXXXX spellings? If we started now, we could be in a better world just a few years from now.

What do you think the GCC folks would say?

@jwakely
Copy link
Contributor

jwakely commented Jul 24, 2024

I asked, and the C++ maintainers would be happy with that direction.

@AaronBallman
Copy link
Collaborator

I prefer the __builtin_ spellings and think that's more clear, and I'd be okay with deprecating the existing spellings and guiding people towards the new ones, but I'm not certain how I feel about introducing new traits with both spellings. Is that the request, or am I misreading?

@ldionne
Copy link
Member

ldionne commented Jul 24, 2024

My thinking was to support both spellings for existing traits only, and introduce new traits as __builtin_xxxxx going forward.

In that case, it looks like we can start right away and this trait should probably be spelled __builtin_is_virtual_base_of.

@AaronBallman
Copy link
Collaborator

Thanks! I think that's a reasonable approach.

@AaronBallman AaronBallman mentioned this issue Jul 25, 2024
Merged
@Endilll
Copy link
Contributor Author

Endilll commented Jul 25, 2024

Given that the decision to use __builtin prefix was made after 19 branch date, are there objections to keep the changes related to this to Clang 20?

@cor3ntin
Copy link
Contributor

__builtin_is_virtual_base_of is Clang 20 material anyway, and the names of previously existing type traits was already agreed upon so we can't change them without being disruptive.

@Endilll
Copy link
Contributor Author

Endilll commented Jul 25, 2024

Technically we can still change __is_layout_compatible name.

@jwakely
Copy link
Contributor

jwakely commented Jul 25, 2024

GCC already defines __is_layout_compatible and libstdc++ uses __has_builtin(__is_layout_compatible) so if Clang adds it with that name, it will Just Work ™️ with existing releases of libstdc++. If you add it with the new name, it won't work unless/until libstdc++ transitions to using the new form of names (which won't happen until GCC starts supporting them with both names).

@Endilll
Copy link
Contributor Author

Endilll commented Jul 25, 2024

Thank you for letting us know. This is strong enough argument to keep __is_layout_compatible spelling.

@ldionne
Copy link
Member

ldionne commented Jul 25, 2024

This seems to be the consensus anyway, but I think we should simply adopt the new __builtin_foo spelling for patches that haven't landed yet. I wouldn't try to rush and change the name of any builtin that we've already landed in the tree but "not released yet". That doesn't seem to be worth it.

cor3ntin pushed a commit that referenced this issue Jul 26, 2024
@Endilll
[clang] Implement __builtin_is_virtual_base_of() intrinsic (#100393)
988fd95 
This patch adds compiler support for
[P2985R0](https://wg21.link/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.
@cor3ntin cor3ntin mentioned this issue Aug 4, 2024
Merged
@MitalAshok MitalAshok mentioned this issue Aug 8, 2024
Merged
@dangelog dangelog mentioned this issue Aug 21, 2024
Closed
hubot pushed a commit to gcc-mirror/gcc that referenced this issue Oct 1, 2024
@dangelog @jicama
c++: introduce __builtin_is_virtual_base_of
1b7cfa7 
P2985R0 (C++26) introduces std::is_virtual_base_of; this is the compiler
builtin that will back up the library trait (which strictly requires
compiler support).

The name has been chosen to match LLVM/MSVC's, as per the discussion
here:

llvm/llvm-project#98310

The actual user-facing type trait in libstdc++ will be added later.

gcc/cp/ChangeLog:

	* constraint.cc (diagnose_trait_expr): New diagnostic.
	* cp-trait.def (IS_VIRTUAL_BASE_OF): New builtin.
	* cp-tree.h (enum base_access_flags): Add a new flag to be
	able to request a search for a virtual base class.
	* cxx-pretty-print.cc (pp_cxx_userdef_literal): Update the
	list of GNU extensions to the grammar.
	* search.cc (struct lookup_base_data_s): Add a field to
	request searching for a virtual base class.
	(dfs_lookup_base): Add the ability to look for a virtual
	base class.
	(lookup_base): Forward the flag to dfs_lookup_base.
	* semantics.cc (trait_expr_value): Implement the builtin
	by calling lookup_base with the new flag.
	(finish_trait_expr): Handle the new builtin.

gcc/ChangeLog:

	* doc/extend.texi: Document the new
	__builtin_is_virtual_base_of builtin; amend the docs for
	__is_base_of.

gcc/testsuite/ChangeLog:

	* g++.dg/ext/is_virtual_base_of.C: New test.
	* g++.dg/ext/is_virtual_base_of_diagnostic.C: New test.

Signed-off-by: Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>
Reviewed-by: Jason Merrill <jason@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Endilll Endilll

Labels
c++26 clang:frontend Language frontend issues, e.g. anything involving "Sema"
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[clang] Implement __is_virtual_base_of() intrinsic Endilll/llvm-project
7 participants
@ldionne @jwakely @cor3ntin @StephanTLavavej @AaronBallman @Endilll @llvmbot