[Pdbg] [RFC][PATCH] WIP: Refactor commands and command argument parsing

[Alistair Popple]

On Wednesday, 30 May 2018 6:57:40 PM AEST Balbir Singh wrote:
> On Wed, May 30, 2018 at 2:26 PM, Alistair Popple <alistair at popple.id.au> wrote:
> > ---
> >  Makefile.am  |   2 +-
> >  src/cfam.c   |  52 ++++++----------------
> >  src/main.c   |  49 ++++++---------------
> >  src/opt.c    | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  src/opt.h    |  96 +++++++++++++++++++++++++++++++++++++++++
> >  src/reg.c    | 137 +++++++++++++++++++---------------------------------------
> >  src/ring.c   |  32 +++-----------
> >  src/scom.c   |  51 +++++-----------------
> >  src/thread.c |  35 ++++++---------
> >  9 files changed, 334 insertions(+), 259 deletions(-)
> >  create mode 100644 src/opt.c
> >  create mode 100644 src/opt.h
> 
> Without a changelog it's really hard to review the
> intention/improvement. Having said that print_cmds_usage() is ugliest
> function I've seen written in all of pdbg (please find a better way to
> do this).

Oh be fair ... I found this yesterday:

	while ((!((val & HTM_MEM_SIZE_SMALL) && i>>20 == 16)) && (!(!(val & HTM_MEM_SIZE_SMALL) && i>>20 == 512))) {

Surely that must be in the running :-)

But yeah, that is something I will cleanup before posting a non-WIP version.

> There's a bit of type dependency in parsing, but I guess
> it's the lack of proper type inference semantics.

Sorry, I should've included a better changelog.

The intent here is to centralise command argument parsing so that we don't have
code like this copied everywhere:

int handle_spr(int optind, int argc, char *argv[])
{
        char *endptr;
        uint64_t spr;

        if (optind + 1 >= argc) {
                printf("%s: command '%s' requires a GPR\n", argv[0], argv[optind]);
                return -1;
        }

        errno = 0;
        spr = strtoull(argv[optind + 1], &endptr, 0);
        if (errno || *endptr != '\0') {
                printf("%s: command '%s' couldn't parse GPR '%s'\n",
                                argv[0], argv[optind], argv[optind + 1]);
                return -1;
        }

Which is not only prone to copy-and-paste bugs (have you spotted the error in
the above?) but also a pain to maintain/change due to the multiple copies. We
could certainly refactor that somewhat to reduce code duplication and that's the
rabbit hole I started down to end up with:

int parse_spr(char *arg)
{
...
}

int getspr(int spr) {
...
};
DEFINE_CMD1(getspr, spr);

Where `spr` specifies the parser for converting the "char *" command argument
into a "int spr" is `parse_spr`. Note that it is in fact type safe as well -
changing parse_spr() to return a long int instead of an int for example will
result in a grokable compiler error.

- Alistair

> Balbir Singh.
>