Hi Paul,
> I'd omit the comment
These comments are necessary for maintenance. When I (or Eric or anyone
else in the future) modifies this code, we need to know which piece of code
applies to which platforms, so that
1. we don't waste time modifying code that has no influence on the
platform on which we are trying to fix a problem,
2. we are reminded to test on all platforms on which our code modifications
are likely to have an effect.
> > +#if _GL_WINDOWS_64_BIT_OFF_T
> > +# undef fseeko
> > +# if HAVE__FSEEKI64 /* msvc, mingw64 */
>
> I'd omit the comment, which was inserted in multiple places, as I find
> it more distracting than helpful. It's better to base code on
> behavior than on names of implementations, and though sometimes the
> latter info needs to leak through this does not appear to be one of
> those places.
But when you modify the code, you have to test it! And for this, you have
to know on which platforms.
This comment also serves as a warning to anyone who thinks "oh, mingw has
fseeko64, so mingw64 certainly also has fseeko64". (Wrong assumption:
mingw64 does not have every function that mingw has.) Likewise it warns
anyone who thinks "_fseeki64 is in MSVCRT, I must be able to call it from
any native Windows platform." (Wrong assumption: The msvcrt import library
on mingw excludes this function.)
> > +/* Needed before <sys/stat.h>.
> > + May also define off_t to a 64-bit type on native Windows. */
> > #include <sys/types.h>
>
> Likewise I'd omit this kind of distracting comment. The comment is
> incomplete: it's not just native Windows where <sys/types.h> has that
> effect.
This comment is a warning: "If you conditionalize this include, think
about testing on native Windows platforms, espectially with the 'largefile'
module." If it's incomplete, please add to it!
> Details like this can be put into the documentation for POSIX headers.
I disagree: The documentation is for the user of gnulib; the comments are
for those who refactor the code.
Bruno
