Page MenuHomePhabricator

[libcxx] Remove _LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED
ClosedPublic

Authored by Mordante on Aug 1 2021, 11:03 AM.

Details

Summary

All supported compilers should support
_LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED so this can be removed.

Diff Detail

Event Timeline

Mordante created this revision.Aug 1 2021, 11:03 AM
Mordante requested review of this revision.Aug 1 2021, 11:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2021, 11:03 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
tschuett added inline comments.
libcxx/include/__config
1295

Could you keep this check as a test to catch unsupported compilers, i.e. move it to the test folder?

Mordante added inline comments.Aug 2 2021, 8:46 AM
libcxx/include/__config
1295

I think it isn't useful to have this in the test folder. This will only catch issues if the user with an unsupported compiler runs the test suite.
If we want to validate the support status it's probably better to do something along the lines of

#if defined(_GNU_VER) && _GNU_VER < 1100
#errror "Only GCC 11 and 12 are supported."
#elif ... // all other supported platforms.

in __config.

Then the unsupported compiler is caught when a user uses it. (Obviously this also catches using it for unit tests.)
WDYT?

tschuett added a comment.EditedAug 2 2021, 8:52 AM

Completely up to you. How about a separate header to catch unsupported compilers?

It would document in code the minimal supported compiler versions. At the moment it is some rst file I guess.

Completely up to you. How about a separate header to catch unsupported compilers?

Not too fond of that idea, __config seems like a good place, it also does other validations.

It would document in code the minimal supported compiler versions. At the moment it is some rst file I guess.

Yes at the moment it's https://libcxx.llvm.org/index.html# under Platform and Compiler Support. So it would be wise to add some comment in the .rst regarding updating the header.

@ldionne how do you feel about validating whether the compiler is supported in __config?

If we want this validation I'd rather do it in a separate patch.

Quuxplusone accepted this revision as: Quuxplusone.Aug 2 2021, 2:34 PM
Quuxplusone added a subscriber: Quuxplusone.

FWIW, this looks good to me. (I was initially prepared to disapprove of removing support for weird old compilers, but after seeing the diff, I agree: any compiler that expects to compile a C++20 standard library implementation needs to provide __builtin_is_constant_evaluated, full stop. And this patch simplifies the codebase nicely.)

Thanks for the review. And indeed the intention is to remove some of the no longer supported compiler work-arounds from libc++.

ldionne accepted this revision.Aug 10 2021, 8:12 AM

LGTM.

The issue of #erroring out on compilers that are not supported is a tricky one, and I'd tackle it as a separate effort. I see the following options:

  1. Error-out on any compiler that isn't officially supported, even if parts of the library may still work.
  2. Error-out only on compilers that we know will fail in crazy ways in order to provide a "fail fast and obviously" kind of experience to users.

The problem with (1) is that it could be perceived as raising the bar for people who want to create a setup that "kinda works". I can easily imagine people saying "my setup works just fine even though you don't officially support it - why are you making my life harder?".

The problem with (2) is that it's a moving target, and it's not easy to define "is obviously broken on compiler version X". For example, GCC 5 *clearly* doesn't work, but what about GCC 10? It's not supported, but parts of the library do work. How broken should compiler support be in order to explicitly #error out in __config?

In both cases, it's not an easy solution. Let's tackle it separately.

This revision is now accepted and ready to land.Aug 10 2021, 8:12 AM

Valid point. Probably GCC-10 will work perfectly when you compile in C++17 mode. Maybe we should not introduce the errors for now and wait and see how many bug reports we get of users using unsupported compilers. If that becomes a burden we can introduce the #errors.

Sorry this may be due to the build bot uses old compiler. I will see if I can upgrade it or use bootstrap.

Sorry this may be due to the build bot uses old compiler. I will see if I can upgrade it or use bootstrap.

This has been discussed off-line. I already had contact with the build-bot maintainer, before I saw this message.