Re: [PATCH 03/24] gdb: make interps_notify work with references

Lancelot SIX
2023-10-11T08:49:28Z

Hi Simon,

Just a nit below.

On Tue, Oct 10, 2023 at 04:39:58PM -0400, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
>
> A subsequent patch changes the interp::on_solib_loaded and
> interp::on_solib_unloaded methods to take references. This highlighted
> that interps_notify did not work with reference parameters.
>
> Fix that by changing interps_notify's `args` arg to be a universal
> reference (&&). Change the type of the method to be auto-deduced as an
> additional template parameter, otherwise the signature of the callback
> function would never match:
>
> CXX interps.o
> /home/simark/src/binutils-gdb/gdb/interps.c: In function ‘void interps_notify_signal_received(gdb_signal)’:
> /home/simark/src/binutils-gdb/gdb/interps.c:378:18: error: no matching function for call to ‘interps_notify(void (interp::*)(gdb_signal), gdb_signal&)’
> 378 | interps_notify (&interp::on_signal_received, sig);
> | ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /home/simark/src/binutils-gdb/gdb/interps.c:363:1: note: candidate: ‘template<class ... Args> void interps_notify(void (interp::*)(Args ...), Args&& ...)’
> 363 | interps_notify (void (interp::*method) (Args...), Args&&... args)
> | ^~~~~~~~~~~~~~
> /home/simark/src/binutils-gdb/gdb/interps.c:363:1: note: template argument deduction/substitution failed:
> /home/simark/src/binutils-gdb/gdb/interps.c:378:18: note: inconsistent parameter pack deduction with ‘gdb_signal’ and ‘gdb_signal&’
> 378 | interps_notify (&interp::on_signal_received, sig);
> | ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Change-Id: I0cd9378e24ef039f30f8e14f054f8d7fb539c838
> ---
> gdb/interps.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/interps.c b/gdb/interps.c
> index f91b2b62c1ba..62a30583e8c0 100644
> --- a/gdb/interps.c
> +++ b/gdb/interps.c
> @@ -358,15 +358,15 @@ current_interpreter (void)
> /* Helper interps_notify_* functions. Call METHOD on the top-level interpreter
> of all UIs. */
>
> -template <typename ...Args>
> +template <typename MethodType, typename ...Args>
> void
> -interps_notify (void (interp::*method) (Args...), Args... args)
> +interps_notify (MethodType method, Args&&... args)
> {
> SWITCH_THRU_ALL_UIS ()
> {
> interp *tli = top_level_interpreter ();
> if (tli != nullptr)
> - (tli->*method) (args...);
> + (tli->*method) (std::forward<Args>(args)...);
^

Space missing before the "(".

Best,
Lancelot.

> }
> }
>
> --
> 2.42.0
>