Re: [PATCH] Fix type errors in win32.

[Eli Zaretskii]

[Please use Reply All to reply, so that the mailing list is CC'ed.]

> From: U2FsdGVkX1 <u2fsdgvkx1@gmail.com>
> Date: Tue, 26 Oct 2021 09:44:01 +0800
> 
> >> From: U2FsdGVkX1 <u2fsdgvkx1@gmail.com>
> >> Date: Mon, 25 Oct 2021 10:55:03 +0800
> >> Cc: U2FsdGVkX1 <U2FsdGVkX1@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.