Re: New module argp-version-etc

[Bruno Haible]

Hi Sergey,

> Here's the updated patch.

I agree that if there is need for two variants, one taking an array and the
other taking a va_list as argument, the one with the array should be the
basic one, because it's easier to convert a va_list to an array than vice
versa.

Regarding version-etc.h:

  - Do you really need *two* array-taking functions? One is not enough?
    IMO it would be better (simpler, easier to understand) if you offer
    just one of version_etc_arn, version_etc_ar. You decide which one.

  - The argument 'const char **authors' should better be a
    'const char * const * authors', because the called function is not
    supposed to modify the contents of the array.

Regarding version-etc.c:

  +/* Like version_etc, below, but with the NULL-terminated author list
  +   provided via a variable of type va_list.  */

  Ouch! Not only you expect the user to look up the documentation of the
  API inside a long source file, but you also play paper chase with the
  user. Really, when you have 2 or 3 functions with similar documentation,
  and want to avoid duplicating that documentation, it is better to move
  that documentation entirely to the .h file. See e.g. lib/xvasprintf.h
  for an example.

Regarding argp-version-etc.h:

  - The function argp_version_setup is lacking a documentation/
    specification. What does it do? What are its arguments? When can it
    be called? What happens if it is called more than once?
  - Again, is the function supposed to modify the argument array? If not,
    change the argument type to 'const char * const *'.

Regarding argp-version-etc.c:

  - The copyright header is missing.

Regarding modules/argp-version-etc:

  - This file is missing (not included in your patch).

Regarding the tests:

  - The file modules/argp-version-etc-tests is missing (not included).

  - It is better to not abbreviate file names. No one will remember, in
    a couple of weeks, what "ave" stands for. Call them
    tests/test-argp-version-etc.*; that follows the common convention
    and is self-explanatory.

  - The test to be run is, AFAIU, tests/test-ave-2.sh. This is the
    first test of this module. It deserves the suffix "-1.sh", not "-2.sh".

  - The '#include "argp-version-etc.h"' should better come as first include
    after the obligatory <config.h>. So that we have a verification that
    the header file is self-contained.

  - 'struct argp test_argp = {' - That's not GNU indentation style. In GNU
    style, the opening brace goes to a new line.

  - 'char *authors[] = {' - Likewise. Also this arrays contains literal
    strings, therefore should have an element type 'const char *', in order
    to avoid gcc warnings in some situations.

  - 'TMP=ave.$$' - Is there any reason for choosing a different file name at
    every run of the test? If not, then it's better to choose a fixed file
    name, e.g. 'ave-expected.tmp'.

  - The statement 'LC_ALL=C' may have no effect if the variable LC_ALL is
    not already defined as an environment variable. To be sure it has an
    effect, follow it with a 'export LC_ALL' statement.

Please commit the two module changes as distinct commits in git.

Thanks!

Bruno