All supported compilers should support
_LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED so this can be removed.
Details
- Reviewers
ldionne • Quuxplusone - Group Reviewers
Restricted Project - Commits
- rG1123100a16a3: [libcxx] Remove _LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/include/__config | ||
---|---|---|
1295 | Could you keep this check as a test to catch unsupported compilers, i.e. move it to the test folder? |
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 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.) |
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.
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.
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++.
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:
- Error-out on any compiler that isn't officially supported, even if parts of the library may still work.
- 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.
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.
This patch caused regression at https://lab.llvm.org/buildbot/#/builders/165/builds/5163
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.
Could you keep this check as a test to catch unsupported compilers, i.e. move it to the test folder?