Re: [PATCH 3/7] gdb: new function to wrap up executing command line scripts/commands

Andrew Burgess
2020-11-02T09:48:46Z

* Aktemur, Tankut Baris <tankut.baris.aktemur@intel.com> [2020-10-08 15:25:25 +0000]:

> On Wednesday, October 7, 2020 10:05 PM, Andrew Burgess wrote:
> > Small refactor to wrap up executing the scripts and commands passed
> > using the -x, -ex, -ix, -iex command line flags.
> >
> > There should be no user visible changes after this commit.
> >
> > gdb/ChangeLog:
> >
> > * main.c (execute_cmdargs): New function.
> > (captured_main_1): Make use of execute_cmdargs.
> > ---
> > gdb/ChangeLog | 5 +++++
> > gdb/main.c | 53 ++++++++++++++++++++-------------------------------
> > 2 files changed, 26 insertions(+), 32 deletions(-)
> >
> > diff --git a/gdb/main.c b/gdb/main.c
> > index f6377f2df28..f85b7dbfdaa 100644
> > --- a/gdb/main.c
> > +++ b/gdb/main.c
> > @@ -515,6 +515,25 @@ struct cmdarg
> > char *string;
> > };
> >
> > +/* From CMDARG_VEC execute command files (matching FILE_TYPE) or commands
> > + (matching CMD_TYPE). Update the value in *RET. */
>
> Is there any particular reason to have the output parameter? The function could have
> returned its result instead of writing it to *ret, right?
>
> > +static void
> > +execute_cmdargs (const std::vector<struct cmdarg> *cmdarg_vec,
> > + cmdarg_kind file_type, cmdarg_kind cmd_type,
> > + int *ret)
> > +{
> > + for (const auto &cmdarg_p : *cmdarg_vec)
> > + {
> > + if (cmdarg_p.type == file_type)
> > + *ret = catch_command_errors (source_script, cmdarg_p.string,
> > + !batch_flag);
> > + else if (cmdarg_p.type == cmd_type)
> > + *ret = catch_command_errors (execute_command, cmdarg_p.string,
> > + !batch_flag);
> > + }
>
> Well, it's the original code that was doing it, but I found it a bit odd that
> ret is being overwritten without checking the value until almost the very end
> of captured_main_1.

I find this odd too, but the last thing I wanted to do was combine a
change in behaviour with a restructuring, so I left things as they
were.

I'll update the comment on the function to (hopefully) document the
existing (odd) behaviour a little better.

Thanks,
Andrew