Re: [PATCH] gdbsupport: add support for references to checked_static_cast

[Simon Marchi <simon.marchi@efficios.com>]

On 5/24/23 11:33, Lancelot SIX wrote:
> On Wed, May 24, 2023 at 10:51:08AM -0400, Simon Marchi via Gdb-patches wrote:
>> On 5/24/23 09:22, Lancelot SIX wrote:
>>> Hi Simon,
>>>
>>>> diff --git a/gdbsupport/gdb-checked-static-cast.h b/gdbsupport/gdb-checked-static-cast.h
>>>> index bc75244bddd0..7e5a69a6474d 100644
>>>> --- a/gdbsupport/gdb-checked-static-cast.h
>>>> +++ b/gdbsupport/gdb-checked-static-cast.h
>>>> @@ -66,6 +66,22 @@ checked_static_cast (V *v)
>>>>    return result;
>>>>  }
>>>>  
>>>> +/* Same as the above, but to cast from a reference type to another.  */
>>>> +
>>>> +template<typename T, typename V>
>>>> +T
>>>> +checked_static_cast (V &v)
>>>> +{
>>>> +  static_assert (std::is_reference<T>::value, "target must be a reference type");
>>>
>>> Have you considered incorporating this constraint inside
>>> checked_static_cast's signature?  It could look like something like
>>> this:
>>>
>>>     template<typename T, typename V>
>>>     typename std::enable_if<std::is_reference<T>::value, T>::type
>>>     checked_static_cast (V &v)
>>>     {
>>>       ...
>>>     }
>>
>> I like static_assert because it's easier to write and read, and I find
>> that the error messages it produces are easier to understand.  They
>> point you directly to the static_assert line that says "target must be a
>> reference", so you know instantly what is wrong.  With template
>> parameters, you have to decrypt the template substitution message, it's
>> really not obvious.  However...
>>
>>> If T is not a reference, then this template won't be instantiated at all
>>> instead of instantiating it and having an error.  This could allow other
>>> templates / functions to be considered if they matched instead of just
>>> producing an error.
>>
>> ... it seems like there are some pragmatic advantages of using the
>> template parameter method, it's probably the right C++ idiom to use,
>> even if I don't like it much.  We have the gdb::Requires util, which
>> makes it at least more straightforward to read the code:
>>
>>     template<typename T, typename V, typename = gdb::Requires<std::is_reference<T>>>
>>
> 
> C++20 would make this more readable:
> 
>     template<typename T, typename V>
>       requires std::is_reference_v<T>
>     T
>     checked_static_cast (V &v) { ... }
> 
> but this is currently not available to us.

Ah nice, it will be easy to convert gdb::Requires to that.

> With the use of gdb::Requires, the static_assert becomes redundant, but
> It does not hurt to keep it.  If you find it easier to capture the
> intent of the code when reading, I do not mind keeping it.

I would remove it, no point in having both (I think the static_assert
would never trigger anyway).

> Feel free to use my RB
> Reviewed-By: Lancelot Six <lancelot.six@amd.com>

Thanks, I'll add it.  Just waiting for the final feedback from Andrew.

Simon