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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131732