[Please use Reply All to reply, so that the mailing list is CC'ed.]
> From: U2FsdGVkX1 <u2fsdgv...@gmail.com>
> Date: Tue, 26 Oct 2021 09:44:01 +0800
>
> >> From: U2FsdGVkX1 <u2fsdgv...@gmail.com>
> >> Date: Mon, 25 Oct 2021 10:55:03 +0800
> >> Cc: U2FsdGVkX1 <u2fsdgv...@gmail.com>
> >>
> >> * src/commands.c (fatal_error_signal): DWORD type should be unsigned
> >> * src/dir.c (print_dir_data_base): Fix format type mismatch
> >> * src/function.c (windows32_openpipe): Use the WINAPI GetStdHandle
> >> function instead
> >> * src/getopt.c (_getopt_internal): Improve code readability
> >
> > Thanks. A couple of comments to some of the hunks you propose:
> >
> >> --- a/src/dir.c
> >> +++ b/src/dir.c
> >> @@ -1119,7 +1119,7 @@ print_dir_data_base (void)
> >> else if (dir->contents->dirfiles.ht_vec == 0)
> >> {
> >> #ifdef WINDOWS32
> >> - printf (_("# %s (key %s, mtime %I64u): could not be
> >> opened.\n"),
> >> + printf (_("# %s (key %s, mtime %llu): could not be
> >> opened.\n"),
> >
> > This change is problematic, because the default C runtime used by Make
> > doesn't support %llu, at least not on Windows versions older than
> > Windows 10. MinGW64 links programs by default with an extension
> > library of its own, which provides ANSI-compatible replacements for
> > printf and stuff, and those do support %llu, but we don't want to make
> > the sources compile only with those tools.
> >
> > Are there any problems with using %I64u there? if so, what problems
> > did you see?
> >
> >> - tmpErr = (HANDLE)_get_osfhandle (errfd);
> >> + tmpErr = GetStdHandle (errfd);
> >
> > This one I don't understand. _get_osfhandle is the documented way of
> > obtaining the OS handle for any CRT file descriptor, so what's wrong
> > with using it here? By contrast, GetStdHandle isn't even explicitly
> > documented to serve this purpose, i.e. to obtain a handle from a CRT
> > file descriptor (since errfd comes from 'fileno'). maybe it can do
> > that, but why use undocumented functionality when a documented one is
> > available?
> >
>
> Hi, thanks for the review!
>
> > - tmpErr = (HANDLE)_get_osfhandle (errfd);
> > + tmpErr = GetStdHandle (errfd);
> Use the GetStdHandle function instead of _get_osfhandle because, in
> Win32 runtime library the _get_osfhandle is defined as
> > intptr_t _get_osfhandle(
> > int fd
> > );
>
> And the GetStdHandle function is defined as
> > // https://docs.microsoft.com/en-us/windows/console/getstdhandle
> > HANDLE WINAPI GetStdHandle(
> > _In_ DWORD nStdHandle
> > );
The fact that 2 functions have the same argument types and return
types doesn't mean they do the same.
> The tmpErr variable can call DuplicateHandle without converting to the
> HANDLE type
>
> And the following also uses the GetStdHandle instead of the
> _get_osfhandle function
> http://git.savannah.gnu.org/cgit/make.git/tree/src/function.c#n1564
We use GetStdHandle in that function when the argument is
STD_INPUT_HANDLE, which is a special fixed value -10. That is, this
is not a real file descriptor, since file descriptors are always
non-negative.
So bottom line, I still don't think we should make this particular
change.
