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

Simple command line argument parsing utility in PJLIB #4013

Merged
merged 6 commits into from
Jul 10, 2024
Merged

Conversation

bennylp
Copy link
Member

@bennylp bennylp commented Jul 9, 2024

The PJSIP suite contains copy of getopt implementation, but it is in PJLIB-UTIL and also has different licensing than PJSIP.

This PR implements a simple, header only argument parsing utilities that can be (and will be) used by pjlib-test and all unit testing apps to replace current manual parsing.

Usage

Argparse provides these few utilities:

  • pj_argparse_exists(..): check if argument exists
  • pj_argparse_get_bool(..): get an argument and remove it from argc/argv if it exists
  • pj_argparse_get_str(..): get an argument and its value and remove the argument-value pair from argc/argv if it exists
  • pj_argparse_get_int(..): get an argument and its int value and remove the argument-value pair from argc/argv if it exists
  • pj_argparse_peek_next_option(..): get first option (argument with preceeding -, or --) in argv, to check if there is remaining unrecognized option

Note

The difference between pj_argparse_exists(..) and pj_argparse_get_bool() is the latter removes the argument from argc/argv if the argument exists.

There is a different semantic between the return value of pj_argparse_get_bool() and pj_argparse_get_str() or pj_argparse_get_int().

pj_argparse_get_bool() returns "the value", i.e. TRUE if the the argument exists.

pj_argparse_get_bool() and pj_argparse_get_str() returns pj_status_t, i.e. PJ_SUCCESS if everything is okay (an argument has been successfully parsed, or the argument does not exist), or error otherwise. In case of error, the error string would have been printed to the screen.

This design decision is chosen for convenience.

Sample

For example, to parse these options:

Usage:
  pjlib-test [OPTION] [test_to_run] [test_to_run]...

where OPTIONS:
  -h, --help             Show this help screen
  -p, --port PORT        Use port PORT for echo port
  -s, --server SERVER    Use SERVER as echo server

test_to_run is the name of the test, e.g. rand_test, os_test, etc.

the following code can be used:

int main(int argc, char *argv[])
{
    int i, port = 0;
    char *server = NULL;

    if (pj_argparse_get_bool(&argc, argv, "-h") ||
        pj_argparse_get_bool(&argc, argv, "--help"))
    {
        usage();
        return 0;
    }

    if (pj_argparse_get_int(&argc, argv, "-p", &port) ||
        pj_argparse_get_int(&argc, argv, "--port", &port))
    {
        usage();
        return 1;
    }

    if (pj_argparse_get_str(&argc, argv, "-s", &server) ||
        pj_argparse_get_str(&argc, argv, "--server", &server))
    {
        usage();
        return 1;
    }

    printf("Request to run %d test(s)\n", argc-1);
    for (i=1; i<argc; ++i)
        printf("  - %s", argv[i]);
    puts("");

    ...
}

Limitations

First, the utility only supports white space(s) as separator between an option and its value. Equal sign is not supported:

# this will not work
$ ./app --server=127.0.0.1

# this works
$ ./app --server 127.0.0.1

Second, the utility does not support double dash (--) as separator between options and operands (see this discussion in stackexchange on the meaning of --)

@bennylp bennylp changed the title Simple argument parsing utility in PJLIB Simple command line argument parsing utility in PJLIB Jul 9, 2024
while (*argv) {
const char *arg = *argv;
if ((*arg=='-' && *(arg+1) && !pj_isdigit(*(arg+1))) ||
(*arg=='-' && *(arg+1)=='-' && *(arg+2)))
Copy link
Member

Choose a reason for hiding this comment

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

according to the spec, *(arg+2) also needs to be a letter, so need to check !pj_isdigit() as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

The comment meant to say that a negative number (e.g. -1) will not be considered as an option. But thinking about it again, and checking the behavior of some programs e.g. gcc or clang, they both consider -1 as an option, e.g.:

$ gcc -1
gcc: error: unrecognized command-line option ‘-1’

So in the latest commit, any arguments that start with dash is considered an option (and comment updated accordingly)

pjlib/include/pj/argparse.h Outdated Show resolved Hide resolved
*
* @return PJ_TRUE if the option exists, else PJ_FALSE.
*/
PJ_INLINE(pj_bool_t) pj_argparse_exists(const char *opt, char *const argv[])
Copy link
Member

Choose a reason for hiding this comment

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

I could be mistaken here, but usually it's the other way around, pj_argparse_exists(argv, opt), with the object being searched (i.e. the main object) being the first parameter.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that's a good point. Rearranged the argument (also in pj_argparse_get_bool()/_str()/_int() to the same convention) in latest commit

}

/**
* Check for an option and if it exists, returns PJ_TRUE remove that option
Copy link
Member

Choose a reason for hiding this comment

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

return PJ_TRUE and remove

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


/**
* Check for an option and if it exists, get the value and remove both
* the option the the value from argc/argv. Note that the function only
Copy link
Member

Choose a reason for hiding this comment

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

and the value

Also below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

/**
* Check for an option and if it exists, get the value and remove both
* the option the the value from argc/argv. Note that the function only
* supports whitespace as separator between option and value (i.e. equal
Copy link
Member

Choose a reason for hiding this comment

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

a single whitespace or whitespaces?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added "limitations" section in this PR and in argparse doxygen to clarify the problem. Also doxygen comments updated to use "whitespace(s)".

@nanangizz
Copy link
Member

pj_argparse_get_bool() and pj_argparse_get_str() returns pj_status_t, i.e. PJ_SUCCESS if everything is okay (an argument has been successfully parsed, or the argument does not exist), or error otherwise. In case of error, the error string would have been printed to the screen.

This design decision is chosen for convenience.

IMHO it is easier to have error returned when argument does not exist because:

  • More intuitive (perhaps subjective) as expected-but-not-exist is usually not a 'success'.
  • App does not need a separate check (using pj_argparse_exists()), especially for int option. Btw, currently the str version also does not seem to return NULL when option does not exist, is this intentional?
  • A bit more efficient because a separate existance check performs an additional loop.

@bennylp
Copy link
Member Author

bennylp commented Jul 10, 2024

(an argument has been successfully parsed, or the argument does not exist), or error otherwise. In case of error, the error string would have been printed to the screen.
This design decision is chosen for convenience.

IMHO it is easier to have error returned when argument does not exist because:

* More intuitive (perhaps subjective) as expected-but-not-exist is usually not a 'success'.

Agreed that this is rather subjective, and it is in my view that since options are by definition optional, it is not wrong to consider lack of it as "success" condition.

But I also understand your point, i.e. if we issue "_get_something()", then one should expect that (s)he gets something or otherwise error should be reported.

In the end, I chose current approach because it is easier to use (we'll discuss more below) and while taking this approach probably is not idealistic in term of "correctness", it's not wrong.

Btw initially the API uses that approach i.e. to return PJ_ENOTFOUND when the option does not exist (see sample here). But this makes using it more verbose, especially when both short and long options are used (not demonstrated in the quoted link).

* App does not need a separate check (using `pj_argparse_exists()`), especially for `int` option. Btw, currently the `str` version also does not seem to return `NULL` when option does not exist, is this intentional?

It is indeed intentional for get_str() and _get_int(), i.e. if the option does not exist then the option value will not be touched. By convention, app should initialize the option value with a default value, which can be used to check if the value is changed by user via cmd line.

Also this allows handling short and long options like this:

    int port = -1;
    
    if (pj_argparse_get_int("-p", &argc, argv, &port) ||
        pj_argparse_get_int("--port", &argc, argv, &port))
    {
        usage();
        return 1;
    }

(if return value is initialized by the function, then the value would have been reset in the second call)

So in most cases, checking if the default value has changed (e.g. not -1 anymore in above sample) can be used instead of pj_argparse_exists().

* A bit more efficient because a separate existance check performs an additional loop.

See above.

@bennylp bennylp merged commit ca43d10 into master Jul 10, 2024
36 checks passed
@bennylp bennylp deleted the argparse branch July 10, 2024 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants