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
