Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch argument parsing to argp #77

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

NickyBoy89
Copy link

This PR converts Spin's manual parsing logic into using the GNU libc extension argp

Pros

This is a large change, and a large diff, but I think this change is beneficial for several reasons:

  1. This removes the maintainence burden of parsing in error-prone cases (manually incrementing argv and argc, pointer arithmetic to parse parts of arguments
  2. Automatic generation of documentation (replaces having to update the usage function manually)
  3. Simpler application logic in some places
  4. Easier testing of the application due to the separation of parsing logic
  5. Reduction of complex control-flow such as goto

Cons

However, this does come with several downsides that I can think of:

  1. Increased application dependencies
  2. Reduced backwards-compatibility, another option, getopt, may be better in this regard
  3. Lack of ability to format documentation manually

Rationale

I think that these pros still outweigh the cons, however, because of the simplification of application logic, and reduced maintenance burden. Originally, I planned to go with the POSIX getopt instead, but it lacked the features for automatic documentation generation, and had more complex parsing logic. I can also revisit this if necessary, if any features are not available in argp.

Safety and compatibility with the existing library

Great care was taken to make sure that this change does not conflict with the existing logic, and can serve as a drop-in replacement. Some of the safety steps taken were:

  1. Enable argp's use of the ARGP_LONG_ONLY flag, which accepts flags containing a single dash (e.g. "-run"), instead of the argp default of just "--run". This is what Spin uses by default, so the parsing of these options should not be accepted
  2. Lines such as this are a bit more complex.
  3. &argv[1][0] refers to the name of the entire argument, e.g. "-Dfoo". However, in argp, the command-line option and argument are parsed separately, requiring a new heap-allocated string to concatenate the option name to the argument. This is safe, and would not likely hurt performance, because parsing only occurs once, and not at runtime, but is something worth considering
  4. PreArg was changed to a helper function that checks an additional assert, and all the call sites were updated to use this variable correctly.
  5. Parsing the options to run was done by setting an internal state flag, which only affects parsing

I hope this can be a welcome inclusion into Spin, or a starting point to another discussion, because I have far more ideas after working on this

~Nick

@@ -303,7 +316,7 @@ alldone(int estatus)
if (cutoff)
{ sprintf(&pan_runtime[strlen(pan_runtime)], "-u%d ", cutoff);
}
for (i = 1; i <= PreCnt; i++)
for (i = 0; i < PreCnt; i++)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above changes to PreArg makes this a zero-indexed array, so every loop of this style needs to be updated. However, this variable is only used in this file, which makes this step easy

assert(PreCnt < sizeof(PreArg));
PreArg[PreCnt++] = arg;
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the previous indexing that used PreArg[++PreCnt] in several places, which I assumed was incorrect, because it makes the array 1-indexed, leaving the first argument unused

@@ -595,7 +608,7 @@ preprocess(char *a, char *b, int a_tmp)

assert(strlen(PreProc) < sizeof(precmd));
strcpy(precmd, PreProc);
for (i = 1; i <= PreCnt; i++)
for (i = 0; i < PreCnt; i++)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above for PreArg

#define OPT_KEY_SIMULATE 6
#define OPT_KEY_SEARCH 7
#define OPT_KEY_S1 8
#define OPT_KEY_S2 9
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These defines are only used as unique values in parsing the long command-line arguments, because argp needs a unique char value to separate everything. These don't need to be printable ASCII, and the short options use their single letters already

printf("spin: bad or missing parameter on -o\n");
usage();
break;
return -1;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error is now handled in the parsing step, but should probably be re-worked because the return value is supposed to be a boolean, and -1 is just a temporary marker value

struct cli_args *args = state->input;

// Handle the run args separately
if (args->parse_run) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having a nested for loop in the parse function to parse arguments after a -run, -search, or others are specified, we set a flag in the parser, and branch to this separate switch statement, which has the same behavior

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant