* [PATCH] gdbsupport: add support for references to checked_static_cast
@ 2023-05-18 20:57 Simon Marchi
2023-05-19 21:31 ` Kevin Buettner
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Simon Marchi @ 2023-05-18 20:57 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
Add a checked_static_cast overload that works with references. A bad
dynamic cast with references throws std::bad_cast, it would be possible
to implement the new overload based on that, but it seemed simpler to
just piggy back off the existing function.
I found some potential uses of this new overload in amd-dbgapi-target.c,
update them to illustrate the use of the new overload. To build
amd-dbgapi-target.c, on needs the amd-dbgapi library, which I don't
expect many people to have. But I have it, and it builds fine here. I
did test the new overload by making a purposely bad cast and it did
catch it.
Change-Id: Id6b6a7db09fe3b4aa43cddb60575ff5f46761e96
---
gdb/amdgpu-tdep.c | 28 +++++++++++++++++++---------
gdbsupport/gdb-checked-static-cast.h | 16 ++++++++++++++++
2 files changed, 35 insertions(+), 9 deletions(-)
diff --git a/gdb/amdgpu-tdep.c b/gdb/amdgpu-tdep.c
index 1077fab5c65e..21a7a3ae1b79 100644
--- a/gdb/amdgpu-tdep.c
+++ b/gdb/amdgpu-tdep.c
@@ -674,7 +674,8 @@ amd_dbgapi_register_type_to_gdb_type (const amd_dbgapi_register_type &type,
case amd_dbgapi_register_type::kind::INTEGER:
{
const auto &integer_type
- = static_cast<const amd_dbgapi_register_type_integer &> (type);
+ = gdb::checked_static_cast<const amd_dbgapi_register_type_integer &>
+ (type);
switch (integer_type.bit_size ())
{
case 32:
@@ -697,7 +698,8 @@ amd_dbgapi_register_type_to_gdb_type (const amd_dbgapi_register_type &type,
case amd_dbgapi_register_type::kind::VECTOR:
{
const auto &vector_type
- = static_cast<const amd_dbgapi_register_type_vector &> (type);
+ = gdb::checked_static_cast<const amd_dbgapi_register_type_vector &>
+ (type);
struct type *element_type
= amd_dbgapi_register_type_to_gdb_type (vector_type.element_type (),
gdbarch);
@@ -716,7 +718,8 @@ amd_dbgapi_register_type_to_gdb_type (const amd_dbgapi_register_type &type,
case amd_dbgapi_register_type::kind::FLAGS:
{
const auto &flags_type
- = static_cast<const amd_dbgapi_register_type_flags &> (type);
+ = gdb::checked_static_cast<const amd_dbgapi_register_type_flags &>
+ (type);
struct type *gdb_type
= arch_flags_type (gdbarch, flags_type.name ().c_str (),
flags_type.bit_size ());
@@ -747,7 +750,8 @@ amd_dbgapi_register_type_to_gdb_type (const amd_dbgapi_register_type &type,
case amd_dbgapi_register_type::kind::ENUM:
{
const auto &enum_type
- = static_cast<const amd_dbgapi_register_type_enum &> (type);
+ = gdb::checked_static_cast<const amd_dbgapi_register_type_enum &>
+ (type);
struct type *gdb_type
= (type_allocator (gdbarch)
.new_type (TYPE_CODE_ENUM, enum_type.bit_size (),
@@ -1310,7 +1314,8 @@ amdgpu_register_type_parse_test ()
gdb_assert (type.kind () == amd_dbgapi_register_type::kind::FLAGS);
- const auto &f = static_cast<const amd_dbgapi_register_type_flags &> (type);
+ const auto &f
+ = gdb::checked_static_cast<const amd_dbgapi_register_type_flags &> (type);
gdb_assert (f.size () == 23);
/* Check the two "FP_ROUND" fields. */
@@ -1322,7 +1327,8 @@ amdgpu_register_type_parse_test ()
== amd_dbgapi_register_type::kind::ENUM);
const auto &e
- = static_cast<const amd_dbgapi_register_type_enum &> (*field.type);
+ = gdb::checked_static_cast<const amd_dbgapi_register_type_enum &>
+ (*field.type);
gdb_assert (e.size () == 4);
gdb_assert (e[0].name == "NEAREST_EVEN");
gdb_assert (e[0].value == 0);
@@ -1338,7 +1344,8 @@ amdgpu_register_type_parse_test ()
gdb_assert (f[22].type->kind () == amd_dbgapi_register_type::kind::INTEGER);
const auto &i
- = static_cast<const amd_dbgapi_register_type_integer &> (*f[22].type);
+ = gdb::checked_static_cast<const amd_dbgapi_register_type_integer &>
+ (*f[22].type);
gdb_assert (i.bit_size () == 32);
gdb_assert (i.is_unsigned ());
}
@@ -1352,13 +1359,16 @@ amdgpu_register_type_parse_test ()
gdb_assert (type.kind () == amd_dbgapi_register_type::kind::VECTOR);
- const auto &v = static_cast<const amd_dbgapi_register_type_vector &> (type);
+ const auto &v
+ = gdb::checked_static_cast<const amd_dbgapi_register_type_vector &>
+ (type);
gdb_assert (v.count () == 64);
const auto &et = v.element_type ();
gdb_assert (et.kind () == amd_dbgapi_register_type::kind::INTEGER);
- const auto &i = static_cast<const amd_dbgapi_register_type_integer &> (et);
+ const auto &i
+ = gdb::checked_static_cast<const amd_dbgapi_register_type_integer &> (et);
gdb_assert (i.bit_size () == 32);
gdb_assert (!i.is_unsigned ());
}
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");
+
+ using T_no_R = typename std::remove_reference<T>::type;
+ using T_P = typename std::add_pointer<T_no_R>::type;
+
+ using V_no_R = typename std::remove_reference<V>::type;
+
+ return *checked_static_cast<T_P, V_no_R> (&v);
+}
+
}
#endif /* COMMON_GDB_CHECKED_DYNAMIC_CAST_H */
base-commit: c96452ad168cf42ad42f0d57214dddb38d5fae88
--
2.40.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] gdbsupport: add support for references to checked_static_cast
2023-05-18 20:57 [PATCH] gdbsupport: add support for references to checked_static_cast Simon Marchi
@ 2023-05-19 21:31 ` Kevin Buettner
2023-05-23 12:03 ` Simon Marchi
2023-05-24 13:07 ` Andrew Burgess
2023-05-24 13:22 ` Lancelot SIX
2 siblings, 1 reply; 11+ messages in thread
From: Kevin Buettner @ 2023-05-19 21:31 UTC (permalink / raw)
To: Simon Marchi via Gdb-patches; +Cc: Simon Marchi
On Thu, 18 May 2023 16:57:37 -0400
Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> wrote:
> Add a checked_static_cast overload that works with references. A bad
> dynamic cast with references throws std::bad_cast, it would be possible
> to implement the new overload based on that, but it seemed simpler to
> just piggy back off the existing function.
>
> I found some potential uses of this new overload in amd-dbgapi-target.c,
> update them to illustrate the use of the new overload. To build
> amd-dbgapi-target.c, on needs the amd-dbgapi library, which I don't
> expect many people to have. But I have it, and it builds fine here. I
> did test the new overload by making a purposely bad cast and it did
> catch it.
I'm not especially knowledgeable in this area, but it LGTM.
Acked-by: Kevin Buettner <kevinb@redhat.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] gdbsupport: add support for references to checked_static_cast
2023-05-19 21:31 ` Kevin Buettner
@ 2023-05-23 12:03 ` Simon Marchi
0 siblings, 0 replies; 11+ messages in thread
From: Simon Marchi @ 2023-05-23 12:03 UTC (permalink / raw)
To: Kevin Buettner, Simon Marchi via Gdb-patches
On 5/19/23 17:31, Kevin Buettner wrote:
> On Thu, 18 May 2023 16:57:37 -0400
> Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> wrote:
>
>> Add a checked_static_cast overload that works with references. A bad
>> dynamic cast with references throws std::bad_cast, it would be possible
>> to implement the new overload based on that, but it seemed simpler to
>> just piggy back off the existing function.
>>
>> I found some potential uses of this new overload in amd-dbgapi-target.c,
>> update them to illustrate the use of the new overload. To build
>> amd-dbgapi-target.c, on needs the amd-dbgapi library, which I don't
>> expect many people to have. But I have it, and it builds fine here. I
>> did test the new overload by making a purposely bad cast and it did
>> catch it.
>
> I'm not especially knowledgeable in this area, but it LGTM.
>
> Acked-by: Kevin Buettner <kevinb@redhat.com>
Thanks, I'll add your Acked-By. I'll wait to see if Andrew has anything
to say about it, since he wrote the original feature.
Simon
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] gdbsupport: add support for references to checked_static_cast
2023-05-18 20:57 [PATCH] gdbsupport: add support for references to checked_static_cast Simon Marchi
2023-05-19 21:31 ` Kevin Buettner
@ 2023-05-24 13:07 ` Andrew Burgess
2023-05-24 14:51 ` Simon Marchi
2023-05-24 13:22 ` Lancelot SIX
2 siblings, 1 reply; 11+ messages in thread
From: Andrew Burgess @ 2023-05-24 13:07 UTC (permalink / raw)
To: Simon Marchi via Gdb-patches, gdb-patches; +Cc: Simon Marchi
Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> Add a checked_static_cast overload that works with references. A bad
> dynamic cast with references throws std::bad_cast, it would be possible
> to implement the new overload based on that, but it seemed simpler to
> just piggy back off the existing function.
>
> I found some potential uses of this new overload in amd-dbgapi-target.c,
> update them to illustrate the use of the new overload. To build
> amd-dbgapi-target.c, on needs the amd-dbgapi library, which I don't
> expect many people to have. But I have it, and it builds fine here. I
> did test the new overload by making a purposely bad cast and it did
> catch it.
Looks great. Thanks for expanding this feature.
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Thanks,
Andrew
>
> Change-Id: Id6b6a7db09fe3b4aa43cddb60575ff5f46761e96
> ---
> gdb/amdgpu-tdep.c | 28 +++++++++++++++++++---------
> gdbsupport/gdb-checked-static-cast.h | 16 ++++++++++++++++
> 2 files changed, 35 insertions(+), 9 deletions(-)
>
> diff --git a/gdb/amdgpu-tdep.c b/gdb/amdgpu-tdep.c
> index 1077fab5c65e..21a7a3ae1b79 100644
> --- a/gdb/amdgpu-tdep.c
> +++ b/gdb/amdgpu-tdep.c
> @@ -674,7 +674,8 @@ amd_dbgapi_register_type_to_gdb_type (const amd_dbgapi_register_type &type,
> case amd_dbgapi_register_type::kind::INTEGER:
> {
> const auto &integer_type
> - = static_cast<const amd_dbgapi_register_type_integer &> (type);
> + = gdb::checked_static_cast<const amd_dbgapi_register_type_integer &>
> + (type);
> switch (integer_type.bit_size ())
> {
> case 32:
> @@ -697,7 +698,8 @@ amd_dbgapi_register_type_to_gdb_type (const amd_dbgapi_register_type &type,
> case amd_dbgapi_register_type::kind::VECTOR:
> {
> const auto &vector_type
> - = static_cast<const amd_dbgapi_register_type_vector &> (type);
> + = gdb::checked_static_cast<const amd_dbgapi_register_type_vector &>
> + (type);
> struct type *element_type
> = amd_dbgapi_register_type_to_gdb_type (vector_type.element_type (),
> gdbarch);
> @@ -716,7 +718,8 @@ amd_dbgapi_register_type_to_gdb_type (const amd_dbgapi_register_type &type,
> case amd_dbgapi_register_type::kind::FLAGS:
> {
> const auto &flags_type
> - = static_cast<const amd_dbgapi_register_type_flags &> (type);
> + = gdb::checked_static_cast<const amd_dbgapi_register_type_flags &>
> + (type);
> struct type *gdb_type
> = arch_flags_type (gdbarch, flags_type.name ().c_str (),
> flags_type.bit_size ());
> @@ -747,7 +750,8 @@ amd_dbgapi_register_type_to_gdb_type (const amd_dbgapi_register_type &type,
> case amd_dbgapi_register_type::kind::ENUM:
> {
> const auto &enum_type
> - = static_cast<const amd_dbgapi_register_type_enum &> (type);
> + = gdb::checked_static_cast<const amd_dbgapi_register_type_enum &>
> + (type);
> struct type *gdb_type
> = (type_allocator (gdbarch)
> .new_type (TYPE_CODE_ENUM, enum_type.bit_size (),
> @@ -1310,7 +1314,8 @@ amdgpu_register_type_parse_test ()
>
> gdb_assert (type.kind () == amd_dbgapi_register_type::kind::FLAGS);
>
> - const auto &f = static_cast<const amd_dbgapi_register_type_flags &> (type);
> + const auto &f
> + = gdb::checked_static_cast<const amd_dbgapi_register_type_flags &> (type);
> gdb_assert (f.size () == 23);
>
> /* Check the two "FP_ROUND" fields. */
> @@ -1322,7 +1327,8 @@ amdgpu_register_type_parse_test ()
> == amd_dbgapi_register_type::kind::ENUM);
>
> const auto &e
> - = static_cast<const amd_dbgapi_register_type_enum &> (*field.type);
> + = gdb::checked_static_cast<const amd_dbgapi_register_type_enum &>
> + (*field.type);
> gdb_assert (e.size () == 4);
> gdb_assert (e[0].name == "NEAREST_EVEN");
> gdb_assert (e[0].value == 0);
> @@ -1338,7 +1344,8 @@ amdgpu_register_type_parse_test ()
> gdb_assert (f[22].type->kind () == amd_dbgapi_register_type::kind::INTEGER);
>
> const auto &i
> - = static_cast<const amd_dbgapi_register_type_integer &> (*f[22].type);
> + = gdb::checked_static_cast<const amd_dbgapi_register_type_integer &>
> + (*f[22].type);
> gdb_assert (i.bit_size () == 32);
> gdb_assert (i.is_unsigned ());
> }
> @@ -1352,13 +1359,16 @@ amdgpu_register_type_parse_test ()
>
> gdb_assert (type.kind () == amd_dbgapi_register_type::kind::VECTOR);
>
> - const auto &v = static_cast<const amd_dbgapi_register_type_vector &> (type);
> + const auto &v
> + = gdb::checked_static_cast<const amd_dbgapi_register_type_vector &>
> + (type);
> gdb_assert (v.count () == 64);
>
> const auto &et = v.element_type ();
> gdb_assert (et.kind () == amd_dbgapi_register_type::kind::INTEGER);
>
> - const auto &i = static_cast<const amd_dbgapi_register_type_integer &> (et);
> + const auto &i
> + = gdb::checked_static_cast<const amd_dbgapi_register_type_integer &> (et);
> gdb_assert (i.bit_size () == 32);
> gdb_assert (!i.is_unsigned ());
> }
> 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");
> +
> + using T_no_R = typename std::remove_reference<T>::type;
> + using T_P = typename std::add_pointer<T_no_R>::type;
> +
> + using V_no_R = typename std::remove_reference<V>::type;
> +
> + return *checked_static_cast<T_P, V_no_R> (&v);
> +}
> +
> }
>
> #endif /* COMMON_GDB_CHECKED_DYNAMIC_CAST_H */
>
> base-commit: c96452ad168cf42ad42f0d57214dddb38d5fae88
> --
> 2.40.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] gdbsupport: add support for references to checked_static_cast
2023-05-24 13:07 ` Andrew Burgess
@ 2023-05-24 14:51 ` Simon Marchi
2023-05-24 18:04 ` Andrew Burgess
0 siblings, 1 reply; 11+ messages in thread
From: Simon Marchi @ 2023-05-24 14:51 UTC (permalink / raw)
To: Andrew Burgess, Simon Marchi via Gdb-patches
On 5/24/23 09:07, Andrew Burgess wrote:
> Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>
>> Add a checked_static_cast overload that works with references. A bad
>> dynamic cast with references throws std::bad_cast, it would be possible
>> to implement the new overload based on that, but it seemed simpler to
>> just piggy back off the existing function.
>>
>> I found some potential uses of this new overload in amd-dbgapi-target.c,
>> update them to illustrate the use of the new overload. To build
>> amd-dbgapi-target.c, on needs the amd-dbgapi library, which I don't
>> expect many people to have. But I have it, and it builds fine here. I
>> did test the new overload by making a purposely bad cast and it did
>> catch it.
>
> Looks great. Thanks for expanding this feature.
>
> Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Thanks. Are you also fine with the switch to use
gdb::Requires<std::is_reference<T> to do the "target is reference"
check, as Lancelot suggested?
Simon
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] gdbsupport: add support for references to checked_static_cast
2023-05-24 14:51 ` Simon Marchi
@ 2023-05-24 18:04 ` Andrew Burgess
2023-05-24 18:54 ` Simon Marchi
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Burgess @ 2023-05-24 18:04 UTC (permalink / raw)
To: Simon Marchi, Simon Marchi via Gdb-patches
Simon Marchi <simon.marchi@efficios.com> writes:
> On 5/24/23 09:07, Andrew Burgess wrote:
>> Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>>
>>> Add a checked_static_cast overload that works with references. A bad
>>> dynamic cast with references throws std::bad_cast, it would be possible
>>> to implement the new overload based on that, but it seemed simpler to
>>> just piggy back off the existing function.
>>>
>>> I found some potential uses of this new overload in amd-dbgapi-target.c,
>>> update them to illustrate the use of the new overload. To build
>>> amd-dbgapi-target.c, on needs the amd-dbgapi library, which I don't
>>> expect many people to have. But I have it, and it builds fine here. I
>>> did test the new overload by making a purposely bad cast and it did
>>> catch it.
>>
>> Looks great. Thanks for expanding this feature.
>>
>> Reviewed-By: Andrew Burgess <aburgess@redhat.com>
>
> Thanks. Are you also fine with the switch to use
> gdb::Requires<std::is_reference<T> to do the "target is reference"
> check, as Lancelot suggested?
Yes, that looks good too.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] gdbsupport: add support for references to checked_static_cast
2023-05-24 18:04 ` Andrew Burgess
@ 2023-05-24 18:54 ` Simon Marchi
0 siblings, 0 replies; 11+ messages in thread
From: Simon Marchi @ 2023-05-24 18:54 UTC (permalink / raw)
To: Andrew Burgess, Simon Marchi via Gdb-patches
On 5/24/23 14:04, Andrew Burgess wrote:
> Simon Marchi <simon.marchi@efficios.com> writes:
>
>> On 5/24/23 09:07, Andrew Burgess wrote:
>>> Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>>>
>>>> Add a checked_static_cast overload that works with references. A bad
>>>> dynamic cast with references throws std::bad_cast, it would be possible
>>>> to implement the new overload based on that, but it seemed simpler to
>>>> just piggy back off the existing function.
>>>>
>>>> I found some potential uses of this new overload in amd-dbgapi-target.c,
>>>> update them to illustrate the use of the new overload. To build
>>>> amd-dbgapi-target.c, on needs the amd-dbgapi library, which I don't
>>>> expect many people to have. But I have it, and it builds fine here. I
>>>> did test the new overload by making a purposely bad cast and it did
>>>> catch it.
>>>
>>> Looks great. Thanks for expanding this feature.
>>>
>>> Reviewed-By: Andrew Burgess <aburgess@redhat.com>
>>
>> Thanks. Are you also fine with the switch to use
>> gdb::Requires<std::is_reference<T> to do the "target is reference"
>> check, as Lancelot suggested?
>
> Yes, that looks good too.
Thanks, pushed.
Simon
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] gdbsupport: add support for references to checked_static_cast
2023-05-18 20:57 [PATCH] gdbsupport: add support for references to checked_static_cast Simon Marchi
2023-05-19 21:31 ` Kevin Buettner
2023-05-24 13:07 ` Andrew Burgess
@ 2023-05-24 13:22 ` Lancelot SIX
2023-05-24 14:51 ` Simon Marchi
2 siblings, 1 reply; 11+ messages in thread
From: Lancelot SIX @ 2023-05-24 13:22 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
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)
{
...
}
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.
I do agree that this can end-up a bit more difficult to read for someone
not used to read this kind of code.
Best,
Lancelot.
> +
> + using T_no_R = typename std::remove_reference<T>::type;
> + using T_P = typename std::add_pointer<T_no_R>::type;
> +
> + using V_no_R = typename std::remove_reference<V>::type;
> +
> + return *checked_static_cast<T_P, V_no_R> (&v);
> +}
> +
> }
>
> #endif /* COMMON_GDB_CHECKED_DYNAMIC_CAST_H */
>
> base-commit: c96452ad168cf42ad42f0d57214dddb38d5fae88
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] gdbsupport: add support for references to checked_static_cast
2023-05-24 13:22 ` Lancelot SIX
@ 2023-05-24 14:51 ` Simon Marchi
2023-05-24 15:33 ` Lancelot SIX
0 siblings, 1 reply; 11+ messages in thread
From: Simon Marchi @ 2023-05-24 14:51 UTC (permalink / raw)
To: Lancelot SIX; +Cc: gdb-patches
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>>>
I'll push it with that change. Should I have your RB?
Simon
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] gdbsupport: add support for references to checked_static_cast
2023-05-24 14:51 ` Simon Marchi
@ 2023-05-24 15:33 ` Lancelot SIX
2023-05-24 15:44 ` Simon Marchi
0 siblings, 1 reply; 11+ messages in thread
From: Lancelot SIX @ 2023-05-24 15:33 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] gdbsupport: add support for references to checked_static_cast
2023-05-24 15:33 ` Lancelot SIX
@ 2023-05-24 15:44 ` Simon Marchi
0 siblings, 0 replies; 11+ messages in thread
From: Simon Marchi @ 2023-05-24 15:44 UTC (permalink / raw)
To: Lancelot SIX; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-05-24 18:54 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-18 20:57 [PATCH] gdbsupport: add support for references to checked_static_cast Simon Marchi
2023-05-19 21:31 ` Kevin Buettner
2023-05-23 12:03 ` Simon Marchi
2023-05-24 13:07 ` Andrew Burgess
2023-05-24 14:51 ` Simon Marchi
2023-05-24 18:04 ` Andrew Burgess
2023-05-24 18:54 ` Simon Marchi
2023-05-24 13:22 ` Lancelot SIX
2023-05-24 14:51 ` Simon Marchi
2023-05-24 15:33 ` Lancelot SIX
2023-05-24 15:44 ` Simon Marchi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).