Re: [PATCH] [cygwin|mingw] Fix compile warnings when -std=c89.

[Charles Wilson]

Ralf Wildenhues wrote:
> I am very sorry that reviewing takes so long.  Mostly this is due to
> time constraints on my side.

On the plus side, your reviews are usually insightful, and lead to ideas
for better code, like the libfile_$(transliterated implib name) thing.

> You can make things easier by splitting them into logically independent
> (hopefully small) pieces.  I acknowledge that some of your other patches
> may not be splittable further.

Part of my tendency to include minor -- easy to review -- changes with
larger ones is due to (a) see it, fix it, otherwise it'll be forgotten
and (b) EVERY separate patchset requires an independent full testsuite
run.  Until recently, that was 5 hours of sitting in front of my
computer waiting for popups, while that computer was completely useless
for anything else (100% cpu).

Not fun. And I don't get many blocks of 5 hours to waste listening for
the Windows Popup Alert sound.  This has little to do with any delays in
reviewing -- but does tend to make the reviewing process harder. And
take longer.  Which means more address@hidden testsuite runs. It's a vicious
cycle, honestly.

Anyway, these 5 hour torture sessions are NOT something I want to do
twice -- or five times -- when once will verify that both (or all five)
separate changes are ok -- but not *necessarily* verify that part #2 is
ok when parts #1, #3, #4, and #5 are not immediately committed along
with it. Even if it seems "obvious" that part #2 is independent.

Recent improvements in cygwin-1.7 have once again tamed the popup
problem on vista -- so at least I don't have to personally attend each
testsuite run. It's still 5 hours of useless computer tho.

MinGW/Msys "native" test runs are still an exercise in whack-a-mole with
the popups.

> This particular patch does two logically separate things:
> 1) repair the creation of the splitted wrapper script.
> 2) reorganize portability macros inside the wrapper C code.
> 
> Part (1) is easy to review: it is obvious that regressions are very
> unlikely to be system-dependent.  One does get the impression that it
> might just be more efficient to let libtool save the cwrapper text
> somewhere and the program just cat that.  But still, this part is ok,
> please apply.

Will do.

Actually, for 2.4 I think the cwrapper should no longer include the
--lt-dump-script option; it is no longer really necessary.  But I'm
extremely hesistant to remove it before then; we've had enough
destabilizing changes in a "stable" release already.

> Part (2) is a bit unobvious when merely looking at the diff.  The
> reorganized lines look ok, but not like a clear improvement from
> the earlier one: both are not fully logical.  And then, why is
> realpath only declared if
>   __CYGWIN__ and __STRICT_ANSI__ && !defined _MSC_VER
> before the patch, but
>   __CYGWIN__ and __STRICT_ANSI__ && !defined __MINGW32__
> after the patch? 

Two reasons: (1) the _MSC_VER was incorrect. It was originally a proxy
for "win32 but not cygwin" -- but it isn't, really. It should have been
#if _MSC_VER || __MINGW32__ (stuff) #else (other stuff) #endif. (2) But
that's really not right either, and characterizing the #ifdef sequence
as you do above is accurate, but misleading. The reason it is misleading
is that things like __MINGW32__, __CYGWIN__, etc are platform defines,
and only one should ever be true at a time (otherwise, you have far
worse issues that we can deal with inside libtool).  Thus:

#if __PLATFORM1__
...stuff-1
#elif __PLATFORM2__
...stuff-2
#elif __PLATFORM3__
...stuff-3
#endif

is really no different than

#if __PLATFORM1__
...stuff-1
#endif
#if __PLATFORM2__
...stuff-2
#endif
#if __PLATFORM3__
...stuff-3
#endif

In the latter case, you wouldn't complain that "stuff-3" only happens
when __PLATFORM3__ && !__PLATFORM2__ && !__PLATFORM1__, even though
(because of the exclusivity of platform macros) it is, in fact, accurate
to say so.  But the former case is effectively the same because of that
same exclusivity -- with one benefit: The former case, but not the
latter, allows a final #else ...default-stuff... clause.

That's the structure I was going for, but it wasn't clear because (a) we
only have three [*] platforms (__MINGW32__, __CYGWIN__, and _MSC_VER as
a proxy for native win32 with msvc compiler) -- but the last case does
not require any function declarations, so it's missing from the
/* declarations of non-ANSI functions */
section. It's hard to see the pattern with only two examples.

[*] counting wince and mingw64, maybe five platforms. but
libtool-developer support for those two is thin.

Technically, the /* portability #defines */ section should be structured
similarly, but I missed that one, so the #if defined(_MSC_VER) ||
defined(__MINGW32__) clause ends with an #endif, not an #elif
__CYGWIN__. But it probably should.  Also, in this section, I tried --
with no actual knowledge of the platform -- to make it easier for the
wince guys to add their hooks.

But I didn't know (and still don't know) if "__MINGW32CE__" is platform
#define, in the exclusivity sense I'm relying on above.  That is, is it
ever possible to have __MINGW32__ && __MINGW32CE__ (outside of
deliberate malicious subversion, obviously)?

Also, it's not clear to me how we should handle -- without lots of
#define duplication -- cases where you should do "X" for platform 1 & 2
but not 3. That's why autoconf-driven things are encouraged to be
feature-specific, not platform specific. But with only -- until very
recently -- two or three platforms to worry about, but 15 or so
features, AND no config.h to use in the cwrapper -- it seemed a bit
counter-productive to go that route.  Maybe it makes more sense, now
that we have 4? 5?-ish platforms to refactor it in that way:
__MINGW32__, __CYGWIN__, msvc, __MINGW32CE__, and MINGW64. (incomplete,
probably very buggy example):

/* at top of cwrapper */
#if __CYGWIN__
# ifdef __STRICT_ANSI__
#  define NEED_REALPATH_DECL
#  define NEED_SETENV_DECL
#  define NEED_PUTENV_DECL
# endif
# define HAVE_SETENV
# define HAVE_UNISTD_H
# define HAVE_IO_H
#elif __MINGW32__
# ifdef __STRICT_ANSI__
#  define NEED__PUTENV_DECL
# endif
# define NEED_SETMODE_TO__SETMODE_DEF /* see note [1] */
# define NEED_STAT_TO__STAT_DEF
# define NEED_CHMOD_TO__CHMOD_DEF
# define NEED_GETCWD_TO__GETCWD_DEF
# define NEED_PUTENV_TO__PUTENV_DEF
# define HAVE_SETMODE
# define HAVE_STAT
# define HAVE_CHMOD
# define HAVE_GETCWD
# define HAVE_PUTENV
#elif __MINGW32CE__
...stuff
#elif _MSC_VER_
...stuff
#else
# error "Platform not supported by libtool cwrapper system"
#endif

And then everything else is feature-driven.

Note [1]: But why is
# define NEED_SETMODE_TO__SETMODE_DEF
inside the #ifdef PLATFORM structure, followed later by
#ifdef NEED_SETMODE_TO__SETMODE_DEF
# define setmode _setmode
#endif
any better than just #defining setmode inside the PLATFORM structure?
Only because one fits better in the config.h/autoconf mold. But we're
not using config.h/autoconf, so...

But anyway, (a) msvc isn't part of master yet, (b) there doesn't seem to
be anyone driving the wince train for libtool, so I'd just be guessing
what should go in that section, and (c) ditto 64-bit.

But refactoring in that way -- by me, blindly -- would *definitely* be a
very large undertaking, and its not clear that it would be a valuable
use of developer resources.

I figured a small amount of change, which made the situation slightly
better if not perfect, /was/ a decent use of my time (because frankly,
the old non-structure "structure" confused ME.)

> FWIW, realpath is used only if !defined S_ISLNK.

You mean if S_ISLNK /is/ defined.

But since realpath is usually declared via the normal #includes, we only
need to forward declare it if __STRICT_ANSI__, AND if we're on a
platform where normally you'd have that function, but __STRICT_ANSI__
means that realpath is excluded from the normal declarations. In our
limited universe of three (or five) platforms, that's __CYGWIN__.  But
on __CYGWIN__, S_ISLNK is always defined, so there's no need to do
#if __CYGWIN__
# if __STRICT_ANSI__
#  if S_ISLNK

> This makes me wince (no pun intended), thinking that just maybe not all
> systems this is relevant have been duly tested (what about MinGW plus
> -std=c89?).

I mentioned in the original post that I had "spot checked" on cygwin --
I didn't say (but should have) that those spot checks were with
-std=c89. You are right tho: I didn't do any "spot checks" with
mingw+std=c89.

> Why is this patch not accompanied by a testsuite addition using
> -std=c89 -Werror on a program that creates a C wrapper?

Because I am well trained to be allergic to making the test suite take
even longer. I know it is not fully rational, because tests help us
avoid these problems in the future. But sweet mother of god is it painful.

--
Chuck