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
