This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] Change exceptions.h functions to use gdb::function_view
- From: Pedro Alves <palves at redhat dot com>
- To: Tom Tromey <tom at tromey dot com>, gdb-patches at sourceware dot org
- Date: Wed, 27 Sep 2017 20:43:20 +0100
- Subject: Re: [RFA] Change exceptions.h functions to use gdb::function_view
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=palves at redhat dot com
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 816B224D0A7
- References: <20170927165302.30177-1-tom@tromey.com>
On 09/27/2017 05:53 PM, Tom Tromey wrote:
> This changes some functions in exceptions.h to use gdb::function_view,
> then fixes up the fallout. This lead to some surprising places, like
> a function pointer in target_so_ops.
>
> While writing this I found that catch_exception_ftype was unused, so I
> removed this.
>
> Note that I did not compile the windows-nat.c change, so I don't know
> if it works.
>
> Regression tested by the buildbot.
>
I'm borderline about this. I have to say that I question the value of
catch_exceptions&co, over just using TRY/CATCH + a scoped_restore(current_uiout)
in the try scope + printing the exception. A TRY/CATCH is likely to be
easier to understand and debug, I think. I mean, take the print_symbol
case [it was just the random one that I picked], and compare:
TRY
{
print_symbol (gdbarch, sym, depth + 1, outfile);
}
CATCH (ex, RETURN_MASK_ERROR)
{
exception_fprintf (gdb_stderr, ex,
"Error printing symbol:\n");
}
vs
catch_errors ([&] ()
{
return print_symbol (gdbarch, sym, depth + 1,
outfile);
},
"Error printing symbol:\n",
RETURN_MASK_ERROR);
It seems like that case doesn't even need a scoped_restore for current_uiout.
And with TRY/CATCH, print_symbol can be simplified further to return void.
Did you consider this?
> - if (catch_exceptions_with_msg (uiout, do_captured_breakpoint_query, &args,
> - error_message, RETURN_MASK_ALL) < 0)
> + if (catch_exceptions_with_msg
> + (uiout,
> + [&] (struct ui_out *)
> + {
> + return do_captured_breakpoint_query (bnum);
> + },
We don't really need the ui_out * parameter in
catch_exceptions_with_msg's callback anymore, since
you can always access it via lambda capture. Would you
consider removing it?
>
> static int
> -do_captured_thread_select (struct ui_out *uiout, void *tidstr_v)
> +do_captured_thread_select (struct ui_out *uiout, const char *tidstr)
> {
> + {
> + return do_captured_thread_select (inner_uiout, tidstr);
> + },
Note the patch has several cases of tabs vs spaces like above.
> @@ -1540,7 +1540,7 @@ get_windows_debug_event (struct target_ops *ops,
> CloseHandle (current_event.u.LoadDll.hFile);
> if (saw_create != 1 || ! windows_initialization_done)
> break;
> - catch_errors (handle_load_dll, NULL, (char *) "", RETURN_MASK_ALL);
> + catch_errors (handle_load_dll, (char *) "", RETURN_MASK_ALL);
> ourstatus->kind = TARGET_WAITKIND_LOADED;
> ourstatus->value.integer = 0;
> thread_id = main_thread_id;
> @@ -1553,7 +1553,7 @@ get_windows_debug_event (struct target_ops *ops,
> "UNLOAD_DLL_DEBUG_EVENT"));
> if (saw_create != 1 || ! windows_initialization_done)
> break;
> - catch_errors (handle_unload_dll, NULL, (char *) "", RETURN_MASK_ALL);
> + catch_errors (handle_unload_dll, (char *) "", RETURN_MASK_ALL);
> ourstatus->kind = TARGET_WAITKIND_LOADED;
> ourstatus->value.integer = 0;
> thread_id = main_thread_id;
I don't think we need the casts nowadays. catch_errors takes a const string (since the -Wwrite-strings patch).
Thanks,
Pedro Alves