Details
- Reviewers
ldionne Mordante var-const - Group Reviewers
Restricted Project - Commits
- rG31ae52859f7a: [libc++] Simplify type_traits and use more builtins
rG42f8f5579897: [libc++] Simplify type_traits and use more builtins
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/include/__type_traits/is_arithmetic.h | ||
---|---|---|
23 | On what compilers is this not supported? | |
libcxx/include/__type_traits/is_compound.h | ||
22 | On what compilers is __is_compound not supported? | |
libcxx/include/__type_traits/is_floating_point.h | ||
22 | Same question! | |
libcxx/include/__type_traits/is_function.h | ||
23–27 | Ditto. | |
libcxx/include/__type_traits/is_fundamental.h | ||
22–23 | The comments are now stale. Also, can we simply assume that we have __is_fundamental? | |
libcxx/include/__type_traits/is_unsigned.h | ||
22–23 | ||
22–24 | I'm pretty sure the latest AppleClang 14 beta is fixed. Once it's out of the beta, we can even drop this conditional altogether since it'll be the only AppleClang version we support (per our policy). |
LGTM, but can you please rebase to poke CI and get it green? I'd like to see this pass on all the compilers + platforms we have.
libcxx/include/__type_traits/is_arithmetic.h | ||
---|---|---|
23 | No, I guess it's OK to keep it as-is. |
Hello! I'm noticing test failures with our downstream Arm compiler that may be related to the changes you're making, but I'm not sure where to begin looking.
I'm seeing the following error:
./include/c++/v1/__functional/invoke.h:14:10: fatal error: '__type_traits/add_lvalue_reference.h' file not found #include <__type_traits/add_lvalue_reference.h> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Did something happen to how add_lvalue_reference.h is installed? Looks like there were changes to the modulemap. Thanks for your help.
add_lvalue_reference has been added in D126244, but I don't think there is anything different from the other granularization patches. Did anything fail like this before?
Nope, it was working fine until a few hours ago. The only files I see actually installed in include/c++/v1/__type_traits/ are "integral_constant.h" and "is_callable.h".
This is causing build breakages in Chrome as we use std::numeric_limits<__float128>.
After this change, std::is_arithmetic<__float128>::value is now true instead of false [0]. Observe that for GCC's libstdc++, the value is also false.
This causes std::numeric_limits<__float128> to fail to compile [1]. This is because std::numeric_limits inherits from __libcpp_numeric_limits, which refers to std::is_arithmetic in its template parameters. The change from false to true causes libc++ to instantiate some illegal template usages of __float128.
[0]: https://godbolt.org/z/EjnY8Th1d Note: at the time of this comment, the output of x86-64 clang(trunk) returned 1 - everything else returns 0.
[1]: https://godbolt.org/z/94PhTsabf Note: at the time of this comment, the output of x86-64 clang(trunk) is https://pastebin.com/w0kRFqB0
[2]: https://github.com/llvm/llvm-project/blob/911841f717eb8acaccf4f3deb5f85fbf6903f55f/libcxx/include/limits#L144-L145
It's not clear to me what you are asking for. Do you think std::is_arithmetic_v<__float128> should return false? Do you want a numeric_limits specialization of __float128?
I don't mean to make a specific recommendation here as I'm not as familiar with libc++. My main point is that this patch causes numeric_limits<__float128> (and very likely many other non-standard types) to fail to build, and that this breakage should be fixed in one way or another. The points I made referencing std::is_arithmetic were to explain why this patch causes the aforementioned build breakage. Whether or not std::is_arithmetic_v<__float128> should return false is something that I'd rather have someone more experienced decide, but I will note that the behavior is now different between libc++ and libstdc++.
This seems like same as the problem that was fixed by https://reviews.llvm.org/rGe6a39f00e8d0cd3684df54fb03d288efe2969202 and re-introduced by this patch.
https://godbolt.org/z/16bhGfM9d
The following code now longer works after this patch:
#include <limits> __float128 test() { return std::numeric_limits<__float128>::min(); }
@ldionne can you please suggest the best course here? I'll note that this same breakage has happened a few years back and at that time a fix was made (https://reviews.llvm.org/rGe6a39f00e8d0cd3684df54fb03d288efe2969202) which is now undone by this change.
is_arithmetic_v<__float128> should be true if __float128 is a compiler-supported floating-point number IMO. It should be noted that __is_integral(__int128) and __is_arithmetic(__int128) are both true, so I don't see why __float128 should be any different.
However, this probably means numeric_limits<__float128> will need a specialisation, if it doesn't already have one.
Unfortunately it seems this revert wasn't followed up by a regression test.
I agree we should fix this and add tests for __float128 and for other non-standard types broken by this commit.
That way we can avoid future regressions.
I think we should also have a look at the gcc/clang manuals for additional non-standard types and evaluate their status.
@ayzhao, @manojgupta, @cjdb are you aware of other types that are broken by this commit?
Nothing to my knowledge, but you may want to double check that __int128 and _BitInt(n) haven't regressed too.
I don't know of any others off of the top of my head, but based on the definition of __is_arithmetic(...), all of the following types are now suspect: https://github.com/llvm/llvm-project/blob/1b24fe34b06cd9f2337313f513a8b19f9a37c5de/clang/include/clang/AST/BuiltinTypes.def#L62-L222
I agree that we should fix the problem, but I don't know how. The right approach seems to be to return true for is_arithmetic_v<__float128> and add a __float128 specialization to numeric_limits. The problem there is that clang doesn't provide the macros to implement it properly. That would mean we have to implement it on a best-effort basis, which doesn't sound great IMO. The other option would be to return false for is_arithmetic_v<__float128>, which is obviously a lie. Neither option sounds great. I think both the current state of is_arithmetic_v<__float128> == true and the old state of is_arithmetic_v<__float128> == false are fundamentally broken. We don't have any tests for either of them, which means that it's a bug waiting to happen.
I will talk to @ldionne about this next week and after that either partially revert this patch and add regression tests or add a specialization to numeric_limits. IMO we shouldn't revert it right now, since it would only change the problem we have and not remove a problem. If you can't wait until Wednesday or Thursday you can always revert the diff downstream.
@ayzhao, @cjdb Thanks for the additional information.
@philnik Is there a conflict in reverting this commit? The general LLVM policy is to keep HEAD in working order. I assume the breakage blocks people from testing HEAD so other patches after this commit are might not tested downstream until HEAD works again.
I'm in favor for a quick (partial) revert, fix it, and re-land the fixed version.
If you can't wait until Wednesday or Thursday you can always revert the diff downstream.
I do not think delaying by a week is a good timeline. I have reverted it for now since this is known to break multiple CIs.
Did the reland in https://reviews.llvm.org/rG31ae52859f7a2339f31bb78c5163c23cb872f179 address the issue raised by Alan and Manoj? The commit message only says "Stuff" :-)
In future, would you mind adding something to describe that the problematic parts have been removed please? It'll make it easier to vet the changes because we'll know what to look for.
On what compilers is this not supported?