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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to