[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added a comment.

In D116203#3435729 <https://reviews.llvm.org/D116203#3435729>, @aaron.ballman wrote:

> In D116203#3434761 <https://reviews.llvm.org/D116203#3434761>, @rjmccall wrote:
>
>> In D116203#3434515 <https://reviews.llvm.org/D116203#3434515>, @cjdb wrote:
>>
>>> In D116203#3431612 <https://reviews.llvm.org/D116203#3431612>, @rjmccall wrote:
>>>
>>>> In D116203#3430332 <https://reviews.llvm.org/D116203#3430332>, @aaron.ballman wrote:
>>>>
>>>>> In D116203#3425512 <https://reviews.llvm.org/D116203#3425512>, @cjdb wrote:
>>>>>
>>>>>> I've noticed that libstdc++ has `using __remove_cv = typename remove_cv<T>::type`, which causes Clang to chuck a wobbly. Changing from `KEYWORD` to `TYPE_TRAIT_1` didn't seem to fix anything.
>>>>>> Is there a way we can work around this, or should we just rename `__remove_cv` and friends to something else?
>>>>>
>>>>> You could work around it by taking note that you're in a libstdc++ system header and do a special dance, but because these are in the implementation's namespace, I think it's probably kinder for everyone to just pick a different name.
>>>
>>> I was hoping we could do something similar to `struct __remove_cv` which would issue a warning?
>>>
>>>>> If you wanted to be especially mean, you could go with `__remove_cvr`, but I'd suggest `__remove_cv_qualifiers` instead. However, what about `restrict` qualifiers? We support them in C++: https://godbolt.org/z/11EPefhjf
>>>>
>>>> Along with a fair number of other vendor qualifiers, yeah.  I think you have to make a policy decision about whether the intent of `std::remove_cv` is really to just remove CV qualifiers or to produce an unqualified type (at the outermost level).  The former is probably more defensible, even if it makes the transform less useful in the presence of extended qualifiers.
>>>
>>> I'm partial to `std::remove_cv` being faithful to its name, unless existing implementations do something else already. I don't mind adding support for the other stuff, but if there's more than just add/remove `restrict`, we're going to have a combinatorial explosion for removes. Is there an alternate way we can approach this?
>>> Possibly:
>>>
>>>   template<class T>
>>>   using remove_const_t = __remove_qualifiers(T, const);
>>>   
>>>   
>>>   template<class T>
>>>   using remove_reference_t = __remove_qualifiers(T, &, &&);
>>>   
>>>   template<class T>
>>>   using remove_rcvref_t = __remove_qualifiers(T, const, volatile, restrict, &, &&); // rcv instead of cvr to prevent a typo with remove_cvref_t
>>
>> I don't think it's worth adding that parsing complexity for a builtin that we expect to only be used in system headers.  Let's just remove `const` and `volatile` and leave other qualifiers in place.
>
> I come down on the opposite side of the fence and think it should remove all qualifiers (or that should be an interface we support in addition to removing just cv qualifiers). WG14 adopted https://www9.open-std.org/jtc1/sc22/wg14/www/docs/n2927.htm into C23 with a `remove_quals` function which removes *all* qualifiers (including `_Atomic`), as an example. But even in C++ mode, I fail to see why we wouldn't want <some> interface for stripping `__restrict` the same as `const` or `volatile` along with some interface for stripping all qualifiers period (I don't see a huge need for a `__remove_cvr` if we have the ability to remove all qualifiers, so I don't think we need the combinatorial explosion or a complex interface). Basically -- if we have extensions like `__restrict` or _Atomic, we should fully support them rather than halfway do it.

Having `__remove_cv` do more than it's advertised to do doesn't sound like a great idea to me. Both libc++ <https://github.com/llvm/llvm-project/blob/main/libcxx/include/type_traits#L647> and libstdc++ <https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/std/type_traits#L1561> define `std::remove_cv` without extensions, and I think it would be surprising for `__remove_cv` to remove anything else.

I'm not against adding `__remove_restrict`, `__remove_qualifiers` (although "qualifier" could imply ref-qualifiers too?), etc.. I suppose that in the rare case someone wants to remove `volatile restrict` and keep `const&`, it's possible to do `__add_const(__remove_qualifiers(T)&)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203