This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Use __is_convertible built-in when available
ClosedPublic

Authored by mcgrathr on Apr 26 2023, 5:30 PM.

Details

Reviewers
philnik
Group Reviewers
Restricted Project
Commits
rG484e64f7e7b2: [libc++] Use __is_convertible built-in when available
Summary

https://github.com/llvm/llvm-project/issues/62396 reports that
GCC 13 barfs on parsing <type_traits> because of the declarations
of struct __is_convertible. In GCC 13, __is_convertible is a
built-in, but __is_convertible_to is not. Clang has both, so
using either should be fine.

Diff Detail

Event Timeline

mcgrathr created this revision.Apr 26 2023, 5:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2023, 5:30 PM
mcgrathr requested review of this revision.Apr 26 2023, 5:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2023, 5:31 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik accepted this revision.Apr 26 2023, 5:45 PM
philnik added a subscriber: philnik.

LGTM % comment with a green CI.

libcxx/include/__type_traits/is_convertible.h
27–32

Please add a //TODO: Remove the #else branch once GCC 13 is used in the CI.

This revision is now accepted and ready to land.Apr 26 2023, 5:45 PM
mcgrathr added inline comments.Apr 26 2023, 7:03 PM
libcxx/include/__type_traits/is_convertible.h
27–32

I'm not clear on what the TODO intends to be done later.
Are you saying that you'll remove the fallback for the case of no built-in at all?
I imagine that would break compatibility with many extant GCC versions that don't have either built-in, wouldn't it?

philnik added inline comments.Apr 26 2023, 8:00 PM
libcxx/include/__type_traits/is_convertible.h
27–32

Yes, that's exactly what I'm saying. We only support the latest released GCC and the last two Clang versions and the one in the same release. People mostly use very recent compiler versions with libc++. Supporting older compilers would be a lot of work for very little benefit.

mcgrathr updated this revision to Diff 517712.Apr 27 2023, 2:21 PM
mcgrathr marked an inline comment as done.

Added TODO comment

libcxx/include/__type_traits/is_convertible.h
27–32

Understood.

This revision was landed with ongoing or failed builds.Apr 27 2023, 2:24 PM
This revision was automatically updated to reflect the committed changes.
shafik added a subscriber: shafik.May 4 2023, 2:32 PM

I think this should have a release note.

I think this should have a release note.

Why? We don't support GCC 13 yet.

shafik added a comment.May 4 2023, 5:36 PM

I think this should have a release note.

Why? We don't support GCC 13 yet.

I must have misunderstood, does this not fix the problem in https://github.com/llvm/llvm-project/issues/62396 even if there is follow-up work todo?

I think this should have a release note.

Why? We don't support GCC 13 yet.

I must have misunderstood, does this not fix the problem in https://github.com/llvm/llvm-project/issues/62396 even if there is follow-up work todo?

Yes, it fixes the problem. But I don't see why we would add a release note. We don't support GCC 13 yet, so it's not unexpected that things break.