Re: [rfc] [0/7] infrun cleanup

[Pedro Alves <pedro at codesourcery dot com>]

On Sunday 07 December 2008 01:28:38, Pedro Alves wrote:
> On Sunday 07 December 2008 00:15:20, Ulrich Weigand wrote:
> 
> > I'd appreciate any feedback on this approach (in particular if 
> > you've done things differently in your attempt) ...
> 
> Eh, no, a lot of dejavu here.  :-)
> 
> > Overall patch set tested on amd64-linux with no regressions.
> 
> Thanks much for doing this.  I've commented on one.  I'll
> look at the rest tomorrow.
> 

It all looks good to me.

The only bit that concerns me, is:

> Within handle_inferior_event (and its subroutines), every path that
> ends in
> 
>    stop_stepping (ecs);
>    return;
> 
> is replaced by
> 
>    return 0;
> 
> and likewise every path that ends in
> 
>    prepare_to_wait (ecs);
>    return
> 
> is replaced by 
> 
>    return 1;

I'm not so sure this makes things clearer than what's there
currently.  One now has to remember what "return 0" or "return 1" means,
while previously, calls to prepare_to_wait/stop_stepping made
it quite explicit.

We also lost the debug message that hinted us that we're going
to need to wait for another target event ("infrun: prepare_to_wait"), or
that we're done ("infrun: stop_stepping").  Perhaps leave the
stop_stopping/prepare_to_wait functions, for the debug output, and
for clarity?

Say, you could make it like so:

-    prepare_to_wait (ecs);
-    return;
+    return prepare_to_wait ();

-    stop_stepping (ecs);
-    return;
+    return stop_stepping ();

And something like:

-static void
-stop_stepping (struct execution_control_state *ecs)
+static int
+stop_stepping (void)
{
  if (debug_infrun)
    fprintf_unfiltered (gdb_stdlog, "infrun: stop_stepping\n");

  /* Let callers know we don't want to wait for the inferior anymore.  */
- ecs->wait_some_more = 0;
+ return 0;
}

Maybe it's just me, though, it's not a strong opinion.

There are a couple of comments left behind that should be cleaned up,
if we remove those functions, e.g.,

/* Print why the inferior has stopped. We always print something when
   the inferior exits, or receives a signal. The rest of the cases are
   dealt with later on in normal_stop() and print_it_typical().  Ideally
   there should be a call to this function from handle_inferior_event()
   each time stop_stepping() is called.*/
static void
print_stop_reason (enum inferior_stop_reason stop_reason, int stop_info)

Or,

  /* Refresh prev_pc value just prior to resuming.  This used to be
     done in stop_stepping, however, setting prev_pc there did not handle
     scenarios such as inferior function calls or returning from
     a function via the return command.  In those cases, the prev_pc
     ...

-- 
Pedro Alves