Hi Akim,
Sorry for the delay.
> Here is my proposal.
It looks really good now. My comments below are only nitpicking.
I like the addition of documentation. It makes the module a lot easier to use.
> +2019-05-23 Akim Demaille <a...@lrde.epita.fr>
> +
> + argmatch: add support to generate the usage message.
> + * lib/argmatch.c: Move some #includes and gettext support to...
> + * lib/argmatch.h: here.
> + (ARGMATCH_DEFINE_GROUP): New macro.
> + * tests/test-argmatch.c (argmatch_backup_docs, argmatch_backup_args)
> + (argmatch_backup_group): New.
> + (CHECK): New.
> + (main): Check argmatch_backup_value, argmatch_backup_xvalue,
> + argmatch_backup_argument and argmatch_backup_usage.
> + * doc/argmatch.texi (Recognizing Option Arguments): New.
The ChangeLog entry should mention that you modify doc/gnulib.texi.
> +@example
> +const argmatch_backup_group_type argmatch_backup_group =
> +@{
> + N_("\
> +The backup suffix is '~', unless set with --suffix or
> SIMPLE_BACKUP_SUFFIX.\n\
> +The version control method may be selected via the --backup option or
> through\n\
> +the VERSION_CONTROL environment variable. Here are the values:\n"),
> + NULL,
> + argmatch_backup_docs,
> + argmatch_backup_args
> +@};
> +@end example
Why does not args element come last and the doc strings come first?
- It'd be more natural for a programmer to put the args element first.
- It at some point in the future, you need to add more doc strings,
it would be easy to add them in a backward-compatible way at the end
of the string (since a struct initializer implicitly initializes
trailing members with NULL or 0 values).
> +ptrdiff_t i = argmatch_backup_value ("--backup", "none");
> +// argmatch_backup_group.args[i].arg is "none", so its value
> +// is argmatch_backup_group.args[i].val.
> +// Return -1 on invalid argument, and -2 on ambiguity.
> +
> +enum backup_type val = *argmatch_backup_xvalue ("--backup", "none");
> +// Returns a pointer to the value, and exit on errors.
> +// So argmatch_backup_group.args[i].val == val.
The naming of the functions is a bit odd. argmatch_backup_xvalue
returns a value, and argmatch_backup_value return not a value but
a choice (an index or -1 or -2). How about renaming these functions:
argmatch_backup_value -> argmatch_backup_choice
argmatch_backup_xvalue -> argmatch_backup_value
> + /* If nonnegative, the indice I of ARG in ARGS, i.e, \
"the indice" is not valid English.
https://www.merriam-webster.com/dictionary/indice
The singular of 'indices' is 'index'.
> + void \
> + argmatch_##Name##_usage (FILE *out) \
> + { \
> + ...
> + if (g->doc_pre) \
> + fprintf (out, "%s\n", _(g->doc_pre)); \
> + int doc_col = argmatch_##Name##_doc_col (); \
Local variable declarations after statements in the same block are a C99
feature. Can you change it to use C99 syntax? Or, if you can't, add 'c99'
to the module dependencies.
Bruno
