Message ID | 1400708905-14184-1-git-send-email-simon.marchi@ericsson.com |
---|---|
State | Changes Requested, archived |
Headers | show |
Hi Simon, > 2014-05-21 Simon Marchi <simon.marchi@ericsson.com> > > * lib/mi-support.exp (mi_run_cmd_full): Add comments. Overall, this looks good to me. I wasn't aware of the history, so just did a little bit of catching up :-), but I confirm that the description you propose matches the actual behavior... > +# use_mi_command selects whether MI or CLI commands are used by the > +# procedure. > +# args is passed to the command used to run the test program, "run" or > +# "-exec-run", depending on the value of use_mi_command. Therefore, if > +# use_mi_command is false, they are effectively args to the test program. > +# If use_mi_command is true, they are simply arguments to -exec-run. > proc mi_run_cmd_full {use_mi_command args} { > global suppress_flag > if { $suppress_flag } { > -- > 2.0.0.rc0 When speaking of the value of a variable, the variable name should be in all caps. For instance: # USE_MI_COMMAND selects whether MI or CLI commands are used by the # procedure. If I may suggest a different approach (anyone is welcome to comment on that too), it'd be useful to succintly say what the function does first, before getting into the details of the parameters. Also, I would group the semantics of each value of use_mi_command together. So, one example: # Send the command to run the test program. # # If USE_MI_COMMAND is true, the "-exec-run" command is used. # Otherwise, the "run" (CLI) command is used. # # ARGS is passed as argument to the command used to run the test program. # Beware that arguments to "-exec-run" do not have the same semantics as # arguments to the "run" command, so USE_MI_COMMAND influences the meaning # of ARGS. If USE_MI_COMMAND is true, they are arguments to -exec-run. # If USE_MI_COMMAND is false, they are effectively arguments passed # to the test program. I hope it helps!
On 14-05-21 06:26 PM, Joel Brobecker wrote: > Hi Simon, > >> 2014-05-21 Simon Marchi <simon.marchi@ericsson.com> >> >> * lib/mi-support.exp (mi_run_cmd_full): Add comments. > > Overall, this looks good to me. I wasn't aware of the history, > so just did a little bit of catching up :-), but I confirm that > the description you propose matches the actual behavior... > >> +# use_mi_command selects whether MI or CLI commands are used by the >> +# procedure. >> +# args is passed to the command used to run the test program, "run" or >> +# "-exec-run", depending on the value of use_mi_command. Therefore, if >> +# use_mi_command is false, they are effectively args to the test program. >> +# If use_mi_command is true, they are simply arguments to -exec-run. >> proc mi_run_cmd_full {use_mi_command args} { >> global suppress_flag >> if { $suppress_flag } { >> -- >> 2.0.0.rc0 > > When speaking of the value of a variable, the variable name should be > in all caps. For instance: > > # USE_MI_COMMAND selects whether MI or CLI commands are used by the > # procedure. Ok. > If I may suggest a different approach (anyone is welcome to comment > on that too), it'd be useful to succintly say what the function does > first, before getting into the details of the parameters. Also, I would > group the semantics of each value of use_mi_command together. So, > one example: > > # Send the command to run the test program. > # > # If USE_MI_COMMAND is true, the "-exec-run" command is used. > # Otherwise, the "run" (CLI) command is used. > # > # ARGS is passed as argument to the command used to run the test program. > # Beware that arguments to "-exec-run" do not have the same semantics as > # arguments to the "run" command, so USE_MI_COMMAND influences the meaning > # of ARGS. If USE_MI_COMMAND is true, they are arguments to -exec-run. > # If USE_MI_COMMAND is false, they are effectively arguments passed > # to the test program. > > I hope it helps! > I think this is very clear. Should I post an updated patch ?
> > If I may suggest a different approach (anyone is welcome to comment > > on that too), it'd be useful to succintly say what the function does > > first, before getting into the details of the parameters. Also, I would > > group the semantics of each value of use_mi_command together. So, > > one example: > > > > # Send the command to run the test program. > > # > > # If USE_MI_COMMAND is true, the "-exec-run" command is used. > > # Otherwise, the "run" (CLI) command is used. > > # > > # ARGS is passed as argument to the command used to run the test program. > > # Beware that arguments to "-exec-run" do not have the same semantics as > > # arguments to the "run" command, so USE_MI_COMMAND influences the meaning > > # of ARGS. If USE_MI_COMMAND is true, they are arguments to -exec-run. > > # If USE_MI_COMMAND is false, they are effectively arguments passed > > # to the test program. > > > > I hope it helps! > > I think this is very clear. Should I post an updated patch ? Sure! :). Watchout for the missing second space after a period in the second to last line.
diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp index 6d011b9..26015dc 100644 --- a/gdb/testsuite/lib/mi-support.exp +++ b/gdb/testsuite/lib/mi-support.exp @@ -806,6 +806,12 @@ proc mi_gdb_test { args } { # In patterns, the newline sequence ``\r\n'' is matched explicitly as # ``.*$'' could swallow up output that we attempt to match elsewhere. +# use_mi_command selects whether MI or CLI commands are used by the +# procedure. +# args is passed to the command used to run the test program, "run" or +# "-exec-run", depending on the value of use_mi_command. Therefore, if +# use_mi_command is false, they are effectively args to the test program. +# If use_mi_command is true, they are simply arguments to -exec-run. proc mi_run_cmd_full {use_mi_command args} { global suppress_flag if { $suppress_flag } {