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

[Lancelot SIX <lsix@lancelotsix.com>]

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.

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.

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

Lancelot.

> I'll push it with that change.  Should I have your RB?
> 
> Simon