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 !...@#$ 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
