Hi Paul,
> I briefly looked at the more recent changes you installed, and have
> some further comments and suggestions and a couple of patches.
>
> * Why two files, msvc-inval.h and msvc-nothrow.h?
> Would it be simpler to have just one file?
> Is it likely that a package would want one but not the other?
> The "AC_DEFUN([gl_MSVC_NOTHROW], [AC_REQUIRE([gl_MSVC_INVAL])])"
> suggests that the two msvc-* modules should be merged, or at
> least that gl_MSVC_NOTHROW and m4/msvc-nothrow.m4 should be removed.
One of the reasons why 'libit' failed but gnulib succeeded is that it was
part of every 'libit's coding conventions that every feature should be
implemented in a single .c file and not more. This required manual work
when accommodating source code such as e.g. libintl. In other words,
the desire to minimize the number of files did not allow the code to be
structured in a good way.
If you are asking to minimize the number of source code files, this goes
against proper design and maintainability.
I chose to make 'msvc-inval' and 'msvc-nothrow' two different files because
they implement two different (but related) layers: 'msvc-inval' provides
the ability to catch invalid parameter notifications, whereas 'msvc-nothrow'
applies this ability to particular functions (currently only one, but maybe
more in the future).
Optimizing m4/msvc-nothrow.m4 is a needless micro-optimization. We have
hundreds of modules that have a part in lib/ and a part in m4/. It's part
of the coding style for gnulib to have a .m4 file, otherwise there is the
tendency to put it into the module description. We have even entirely no-op
.m4 files, like m4/i-ring.m4 or m4/xgetcwd.m4.
It is more important to have the room for new code to be added in the proper
places, than to reduce the number of files.
> * Why is the body of msvc-inval.c protected inside
> HAVE_MSVC_INVALID_PARAMETER_HANDLER? Surely that file is compiled
> only when the macro is true. There's a similar issue for
> msvc-nothrow.c.
Yes, this #if is technically redundant, because the 'if test ...'
in the module description avoids a completely empty compilation unit
anyway. I put in the #if so that the structure of the .c file is similar
to the structure of the .h file.
> * Why does dup2.c need to use 'inline'? When a static function is
> used in only one place, 'inline' is typically verbosity that's not
> needed: a compiler that's optimizing will inline anyway, and if
> we're not optimizing, then typically we won't want the inlining
> (for debugging purposes). A similar comment applies to other
> *_nothrow functions.
Do you know the optimizing behaviour of MSVC, or are you making assumptions?
What I found out is that with MSVC 9, inlining is triggered by the
"global optimizations" switch, and for small static functions, it does it
also without the programmer specifying 'inline'. What I also know from MSVC
4/5/6, is that this "global optimizations" switch leads to compiler bugs.
I don't know if there is another (reliable) optimization switch for MSVC 9
that would enable inlining.
So, it may be that you have a point here and that the 'inline's are
unnecessary. But I don't have enough knowledge about MSVC compilers to
evaluate that.
> * dup2.c can benefit from more reorganization to make it shorter and
> easier to follow (especially for us non-Windows guys). The basic
> idea is to put the Windows-only stuff into a separate section,
> easily delimited via an #if. I took a stab at doing that (see
> below), and the result is shorter (and to my eyes) clearer
It is clearer, yes. But your patch has three problems:
* It contains the code
if (fd == desired_fd)
return fcntl (fd, F_GETFL) == -1 ? -1 : fd;
unprotected by #ifs, which will lead to compilation failure on native
Windows platforms. They don't have fcntl() nor F_GETFL. gnulib defines
fcntl() on these platforms, but the 'dup2' module does not depend on
the 'fcntl' module.
* Cygwin is completely mishandled.
Fact #1: On Cygwin, i.e. when creating Cygwin executables on Cygwin,
(defined _WIN32 || defined __WIN32__) may be true or may be false;
it depends on the compilation options (-mwin32 vs. -mno-win32). But
regardless of these options, the program will be using the libc API from
cygwin.dll, not msvcrt.dll.
Fact #2: Cygwin programs should normally *not* use <windows.h> and the
Win32 API. This is explicit policy (see
<http://www.cygwin.org/ml/cygwin/2011-09/msg00065.html>). It also
reflects reality better: For a C programmer, Cygwin feels 90% like POSIX
and 10% like Windows.
As a consequence, never write
#if defined _WIN32 || defined __WIN32__
Always write
#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
if you want to treat Cygwin like POSIX (which is the common case).
Or
#if (defined _WIN32 || defined __WIN32__) || defined __CYGWIN__
if you want to treat Cygwin like native Windows (which is pretty rare).
* It adds a wrong comment:
/* Cygwin 1.5.x dup2 (1, 1) returns 0. */
if (result == 0)
result = desired_fd;
Compare the code with doc/posix-functions/dup2.texi, you will notice that
these two lines are needed for mingw and MSVC platforms, not for Cygwin.
> I haven't tested the new dup2.c under Windows
If you want, I can provide you ssh access to a Windows machine with Cygwin and
mingw installed and ready to use.
I have tested this patch on Cygwin and mingw.
2011-09-24 Bruno Haible <br...@clisp.org>
dup2: Fix last commit.
* lib/dup2.c: Restore comments. Treat Cygwin like Unix.
(rpl_dup2): Disable fcntl workaround on native Windows.
--- lib/dup2.c.orig Sat Sep 24 13:50:50 2011
+++ lib/dup2.c Sat Sep 24 13:43:00 2011
@@ -29,20 +29,22 @@
# undef dup2
-# if defined _WIN32 || defined __WIN32__
+# if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
+
+/* Get declarations of the Win32 API functions. */
+# define WIN32_LEAN_AND_MEAN
+# include <windows.h>
+
# include "msvc-inval.h"
-# ifndef __CYGWIN__
-# define WIN32_LEAN_AND_MEAN
-# include <windows.h>
-# include "msvc-nothrow.h"
-# endif
+
+/* Get _get_osfhandle. */
+# include "msvc-nothrow.h"
static inline int
ms_windows_dup2 (int fd, int desired_fd)
{
int result;
-# ifndef __CYGWIN__
/* If fd is closed, mingw hangs on dup2 (fd, fd). If fd is open,
dup2 (fd, fd) returns 0, but all further attempts to use fd in
future dup2 calls will hang. */
@@ -55,7 +57,6 @@
}
return fd;
}
-# endif
/* Wine 1.0.1 return 0 when desired_fd is negative but not -1:
http://bugs.winehq.org/show_bug.cgi?id=21289 */
@@ -76,13 +77,14 @@
}
DONE_MSVC_INVAL;
- /* Cygwin 1.5.x dup2 (1, 1) returns 0. */
if (result == 0)
result = desired_fd;
return result;
}
+
# define dup2 ms_windows_dup2
+
# endif
int
@@ -90,10 +92,13 @@
{
int result;
+# if !((defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__)
/* On Linux kernels 2.6.26-2.6.29, dup2 (fd, fd) returns -EBADF.
+ On Cygwin 1.5.x, dup2 (1, 1) returns 0.
On Haiku, dup2 (fd, fd) mistakenly clears FD_CLOEXEC. */
if (fd == desired_fd)
return fcntl (fd, F_GETFL) == -1 ? -1 : fd;
+# endif
result = dup2 (fd, desired_fd);
--
In memoriam Sara Harpman
<http://www.genealogieonline.nl/en/stamboom-harpman/I399.php>
