Re: argmatch: accept perfect matches in documented arglists

[Bruno Haible <bruno@clisp.org>]

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  <akim@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