[libcxx-commits] [PATCH] D131732: [libcxx][NFC] utilises compiler builtins for unary transform type-traits

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added a subscriber: jwakely.
cjdb added a comment.

In D131732#3751943 <https://reviews.llvm.org/D131732#3751943>, @philnik wrote:

> In D131732#3751895 <https://reviews.llvm.org/D131732#3751895>, @cjdb wrote:
>
>> In D131732#3750935 <https://reviews.llvm.org/D131732#3750935>, @rupprecht wrote:
>>
>>> 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 <https://reviews.llvm.org/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:
>>
>> 1. 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.
>> 2. 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 GCC side of things please?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131732/new/

https://reviews.llvm.org/D131732