Re: New module argp-version-etc

[Eric Blake]

Sergey Poznyakoff <gray <at> gnu.org.ua> writes:

> > Why'd you drop the comments describing what the method does?
> 
> I did not. I simply retained the original comment before version_etc_va.
> I should have supplied comments before the two new functions, that's
> true. I'll fix this.

I see now.  It was just a matter of how the diff lined up, due to moving the 
bulk of the body to the new function, while keeping the old function (and 
documentation) as a wrapper around the new.  But yes, adding comments to the 
new functions will be appreciated.

> > > +  for (n_authors = 0;
> > > +       n_authors < 10
> > > +  && (authtab[n_authors++] = va_arg (authors, const char *)) != NULL;
> > > +       n_authors++)
> > > +    ;
> > 
> > missing va_end
> 
> Is it really necessary here? I believe the caller should call
> va_end, because that's the caller who called va_start. Am I missing
> something?

va_end is often a no-op.  But that does not excuse us from using it - to be 
correct, every va_start/va_copy should have a corresponding va_end, although 
not necessarily in the same function; and it is always a good idea when writing 
a va_list function to document whether the caller is responsible for freeing 
the va_list or whether the callee will consume it.

Thus, the state of the code prior to your patch was buggy - we had va_start
(authors) in version_etc matched with va_end(authors) in version_etc_va, but an 
unmatched va_copy(tmp_authors) with no va_end in version_etc_va, and no 
documentation in version_etc_va on which semantics were in effect.

Your idea of making the caller free the va_args is fine, but that means that 
version_etc now needs the va_end(authors) that was deleted out of 
version_etc_va.  And by deleting the va_copy(tmp_authors), we no longer have to 
worry about supplying the va_end(tmp_authors).  It is a slight semantics change 
(direct callers of version_etc_va are now responsible for cleanup), a code bug 
fix (callers of version_etc_va no longer leak in the case where va_end is more 
substantive than a no-op), and a documentation bug fix (version_etc_va will now 
clearly call out who has the responsibility to call va_free).

> > Are we going to have to update this test every year?  It would be nice to 
> > compute the year that should be present, rather than hard-coding it
> 
> I thought about it too. But there is no warranty the computed year will
> coincide with the COPYRIGHT_YEAR constant from version-etc.c.

One alternative is to massage the actual output through sed to match the 
expected output, regardless of the year from version-etc.c.  Such as:

./test-ave --version | sed 's/(C) [0-9][0-9][0-9][0-9]/(C) 2009/' \
  | diff -c $TMP - || ERR=1 

-- 
Eric Blake