Re: [RFA] Reverse debugging, part 3/3: user interface / docs

[Eli Zaretskii <eliz@gnu.org>]

> Date: Fri, 31 Mar 2006 14:19:05 -0800
> From: Michael Snyder <msnyder@redhat.com>
> 
> +   switch (dir) {
> +   case EXEC_FORWARD:
> +     fprintf_filtered (out, "Forward.\n");
> +     break;
> +   case EXEC_REVERSE:
> +     fprintf_filtered (out, "Reverse.\n");
> +     break;
> +   case EXEC_ERROR:
> +   default:
> +     error (_("Target `%s' does not support execution-direction."),
> + 	   target_shortname);
> +     break;
> +   }

Did you really mean for "Forward" and "Reverse" to be untranslated (as
opposed to the error text)?  If so, please explain why.  If not,
please add _().

> + }
> + 
> + /* User interface:
> +    reverse-step, reverse-next etc.
> +    (returns error unles target implements to_set_execdir method).  */
> + 
> + static void execdir_default (void *notused)
> + {
> +   /* Return execution direction to default state.  */
> +   target_set_execution_direction (EXEC_FORWARD);
> + }
> + 
> + static void
> + exec_reverse_once (char *cmd, char *args, int from_tty)
> + {
> +   /* String buffer for command consing.  */
> +   char reverse_command[512];
> [...]
> +   sprintf (reverse_command, "%s %s", cmd, args ? args : "");

Don't we have xstrprintf and friends to avoid buffers with arbitrary
sizes?

> + void
> + _initialize_reverse (void)
> + {
> +   add_setshow_enum_cmd ("exec-direction", class_run, exec_direction_names,
> + 			&exec_direction, "Set direction of execution.\n\
> + Options are 'forward' or 'reverse'.",
> + 			"Show direction of execution (forward/reverse).",
> + 			"Tells gdb whether to execute forward or backward.",
> + 			set_exec_direction_func, show_exec_direction_func,
> + 			&setlist, &showlist);

The doc strings should be wrapped in _().

Also, I think "Tell" is more consistent with the rest of doc strings
than "Tells".

> + @node Reverse Execution
> + @chapter Running programs backward

Please add "@cindex reverse execution" here.

> + If the target environment supports it, @value{GDBN} can allow you to
> + ``rewind'' the program by running it backward.

"GDB allows you to ``rewind'' the program...".  ``Can allow'' implies
that you need to do something before it allows.

> + program was executing normally.  Variables, registers etc. should 

"etc.@:", since the period doesn't end a sentence.

> +                                                 Behavior of 
> + asynchronous signals depends on the target environment.

This is too vague.  Can we at least mention the possible behaviors, or
just the most common/expected ones?  The reader should get some idea
of what might happen.

> + @item reverse-step @r{[}@var{count}@r{]}
> + Run the program backward until control reaches the start of a
> + different source line

Isn't it better to say

  Run the program backwards until control reaches the first instruction
  of a different source line

?  In any case, "backwards", not "backward".

; then stop it, and return control to @value{GDBN}.
> + 
> + Like the @code{step} command, @code{reverse-step} will only stop
> + at the beginning of a source line.  It ``un-executes'' the previously
> + executed source line.  If the previous source line included calls to 
> + debuggable functions, @code{reverse-step} will step (backward) into
> + the called function, stopping at the beginning of the @emph{last}
> + statement in the called function (typically a return statement).

This ``last statement'' is not entirely accurate, I think.  What
happens in a function that has several return statements? we will wind
up at the one thru which it returned on the way forward, right?

> + @item set exec-direction 
> + Set the direction of target execution.
> + @itemx set exec-direction reverse

Please add a @kindex entry for this command.