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

[Balbir Singh]

On Thu, May 31, 2018 at 11:56 AM, Alistair Popple <alistair at popple.id.au> wrote:
> 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 :-)
>

Surely that wins, sigh!

> 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.
>

That's much better, yeah we probably just need a type converter from
string to type T, I suppose. I still don't like the ACTION1/2, seems
irrelavant to the cause.

Balbir