Re: [RFA] Change exceptions.h functions to use gdb::function_view

[Pedro Alves <palves at redhat dot 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