Depends on D116203
Details
- Reviewers
ldionne philnik - Group Reviewers
Restricted Project - Commits
- rG0e7971154ecb: [libcxx][NFC] utilises compiler builtins for unary transform type-traits
rG06a1d917ef1f: [libcxx][NFC] utilises compiler builtins for unary transform type-traits
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Which compiler supports these builtins? Neither GCC nor clang accept __add_lvalue_reference or __add_pointer: https://godbolt.org/z/nvxqWTGY1
Could you rebase on top of D116203 to run the clang patch with this patch together in the CI? I think it would be good to land these two patches at the same time.
Do you have any benchmarks to show how much of a difference it makes?
libcxx/include/__type_traits/add_lvalue_reference.h | ||
---|---|---|
21–30 | For all of the changes: Please clang-format them and use using instead of typedef. | |
libcxx/include/__type_traits/decay.h | ||
64 | Please also add a comment to other longer preprocessor conditions. | |
libcxx/include/__type_traits/is_referenceable.h | ||
23 | I think a better name would be __libcpp_is_referenceable. __referenceable sounds a lot like a concept, not a type trait. | |
libcxx/include/__type_traits/remove_cvref.h | ||
30 | This is just a duplicate of line 43. Could you move them out of the #if __has_builtin() instead? |
The commit says NFC, but we found this to be an observable behavior change with __restrict now being dropped as part of std::decay: https://godbolt.org/z/zqvW478jq. It impacted us via use of std::make_pair which decays the types, and we had a static_assert checking that the type passed in had __restrict on it.
We have a workaround (just don't use std::make_pair), but I figured I should mention this since the commit is labelled NFC, so any observable change is a surprise.
IIUC, because __restrict is non-standard C++, it's implementation defined whether std::decay wants to modify it or not. Therefore, both before and after are "correct", and code should not rely on which option we choose.
Thanks for flagging this.
It was a deliberate choice to have __decay drop all CVR qualifiers in D116203, with the rationale being that if C++ supported restrict, it would probably be affected by std::decay. The libc++ change was supposed to be NFC, but I'd forgotten about the strictness of __decay by the time I'd gotten around to changing libc++ code.
I see two paths forward:
- Wait to see if any libc++ maintainers have opinions regarding __restrict. I don't think that they have up until now, since their CI would've caught it otherwise; though they might want to retroactively have opinions now that we know people have been negatively impacted by this.
- Immediately patch Clang so that __decay doesn't impact __restrict, since people have been negatively impacted by this change.
#include <type_traits> int main() { int* __restrict const volatile a = nullptr; auto v = a; static_assert(std::is_same_v<decltype(v), int*>); }
Clang says that __restrict is dropped here, so I think it should also be dropped when using std::decay. @cjdb could you make a follow-up to also fix this for GCC and add tests?
I can add tests for libc++, but I'm not familiar with the GCC codebase. @jwakely, do you have any opinions on this particular change, and if you're of a similar mind, would you mind helping out on the libstdc++ side of things please?
@cjdb I meant that the other decay implementation should be fixed to be the same as the builtin. IOW the one that GCC uses, since it doesn't have the builtin. Sorry for the confusion.
Thanks for clarifying. After double checking the Clang test, I see that __decay strips all qualifiers: not just __restrict. For example, int* _Nonnull will become int*. There are a lots of these, and I'm not sure how we'd want to handle this without ironically impacting build times.
For all of the changes: Please clang-format them and use using instead of typedef.