GNOME Bugzilla – Bug 688109
win32 warning/error fixes
Last modified: 2012-11-16 13:54:11 UTC
Apparently glib has not been buildable on win32 since Colin added all those -Werror=foo flags, and no one noticed... Oh, there's one actual bugfix in here too (the GLocalFile patch). Tested that this doesn't add any new warnings or break "make check" on unix.
Created attachment 228713 [details] [review] win32: prototype the binary-compatibility functions For the various functions where (on win32) the "real" version is now foo_utf8, but the old version is left around for binary compatibility, we need to re-prototype them after #undef'ing the foo_utf8 renaming, so that gcc -Werror=missing-prototypes won't error out.
Created attachment 228714 [details] [review] win32: prototype _glib_get_dll_directory() and _glib_get_locale_dir() Rather than using "extern" declarations of these win32 functions everywhere they're needed, just prototype them in glib-private.h. (Which also fixes the fact that they weren't prototyped in the files where they're defined.)
Created attachment 228715 [details] [review] win32: define _WIN32_WINNT globally Rather than defining _WIN32_WINNT only in a handful of files, define it in config.h, like we do with _GNU_SOURCE. (Also remove a "#define WIN32_LEAN_AND_MEAN" that isn't really all that useful.)
Created attachment 228716 [details] [review] win32: move some code into #ifdef G_OS_UNIX Fix various bits of code/declarations that are only used by G_OS_UNIX but were still visible to G_OS_WIN32.
Created attachment 228717 [details] [review] win32: misc warning fixes
Created attachment 228718 [details] [review] win32: various fixes to test programs Fix a few win32-specific bugs in various tests, and #ifdef out code that is UNIX- or Linux-specific that wouldn't be expected to pass on Windows.
Created attachment 228719 [details] [review] GLocalFile: canonicalize the initial directory separator GLocalFile was (in certain situations) translating a path like "/foo/bar/baz" to "/foo\bar\baz" on win32. Fix it to make sure the initial directory separator gets canonicalized too.
Created attachment 228720 [details] [review] gvariant-internal.h: fix the include hack gvariant-internal.h was defining GLIB_COMPILATION so that it could include individual headers, but this broke tests/gvariant on windows because setting GLIB_COMPILATION changes the definition of GLIB_VAR, causing external variables to not be found. Fix this by having it define __GLIB_H_INSIDE__ instead.
Created attachment 228721 [details] [review] win32: make gio/tests/gdbus-proxy.c compile win32 doesn't have kill(), so this won't even compile on Windows unless that is ifdeffed out. The test probably still doesn't *work*, but...
Created attachment 228722 [details] [review] win32: avoid printf format warnings on nonstandard formats glib/tests/test-printf tests some non-standard printf formats on Windows, which gcc doesn't recognize, and so complains about. Disable those warnings for that test.
Created attachment 228723 [details] [review] win32: work around broken winsock prototypes Re-#define a few socket functions to work around winsock's prototypes having, eg, "int *" rather than "unsigned int *", or "char *" rather than "void *". (Also fix two places that mistakenly assumed guint==guint32.)
Created attachment 228724 [details] [review] gtestutils: don't try to print long doubles A few gtestutils function use long double as a type that can (in theory) hold any int or any double. But win32 doesn't support long doubles in printf, so convert them to ints or doubles first before trying to print them.
Colin, this is all your fault for giving us better warnings, so I nominate you to review the patches. Though failing that, I will eventually just commit them unreviewed, since win32 patches have a tendency to sit around in bugzilla forever otherwise. (In particular though, "win32: prototype the binary-compatibility functions" is a general code style question, "win32: various fixes to test programs" may want some sanity-checking on if we really do want to disable certain tests on Windows, "GLocalFile: canonicalize the initial directory separator" is an actual bugfix [as opposed to just warning fix], and could potentially affect unix if it was incorrect, and "gtestutils: don't try to print long doubles" is a slight behavior change that some people might consider incorrect.) Also, if any win32 types are reading this, note that there are still some warnings even with all of these patches, which I can't convince myself aren't possible bugs.
Review of attachment 228713 [details] [review]: Oh wow...sorry for this pain. Hrm...this is a lot of duplication. Would it be possible to carry prototypes for both the _utf8 and non-utf8 version in the headers? Would require inverting the conditionals so that #ifdef G_OS_WIN32 void g_foo_utf8 (...) #endif Otherwise, let's just commit this though and get the build working again.
Review of attachment 228714 [details] [review]: Looks right.
Review of attachment 228715 [details] [review]: There's some 500 versus 501 going on here; ok, http://msdn.microsoft.com/en-us/library/windows/desktop/aa383745(v=vs.85).aspx says 501 is right.
Review of attachment 228716 [details] [review]: Looks right.
Review of attachment 228717 [details] [review]: Minor things, good to commit regardless. ::: gio/gregistrysettingsbackend.c @@ -1911,3 @@ -GSettingsBackend * -g_registry_backend_new (void) { Hm...but was this exported? ::: gio/gwin32mount.c @@ +110,1 @@ _win32_get_displayname (const char *drive) Will these have been exported on Windows too? ::: gio/gwin32volumemonitor.c @@ -42,3 @@ - - GList *volumes; - GList *mounts; So this was always just a stub class? Ok. ::: glib/gkeyfile.c @@ +48,1 @@ #define stat _stati64 Eww? Seems like these bits should live somewhere else, but not your fault...
Review of attachment 228718 [details] [review]: ::: glib/tests/uri.c @@ +259,3 @@ b = ""; #ifndef G_OS_WIN32 + return strcmp (a, b); Why change g_strcmp0 to strcmp?
Review of attachment 228719 [details] [review]: Seems like this one could use a test?
(In reply to comment #16) > Review of attachment 228715 [details] [review]: > > There's some 500 versus 501 going on here; ok, > http://msdn.microsoft.com/en-us/library/windows/desktop/aa383745(v=vs.85).aspx > says 501 is right. It's not so much about "right"; basically 500 means -DWIN32_VERSION_MIN_REQUIRED=WINDOWS_2000 and 501 means -DWIN32_VERSION_MIN_REQUIRED=WINDOWS_XP. So if we're using 501 anywhere, then we might as well be using it everywhere. (In reply to comment #18) > -GSettingsBackend * > -g_registry_backend_new (void) { > > Hm...but was this exported? nope, not in gio.symbols > Will these have been exported on Windows too? likewise > So this was always just a stub class? Ok. apparently (In reply to comment #19) > Review of attachment 228718 [details] [review]: > > ::: glib/tests/uri.c > @@ +259,3 @@ > b = ""; > #ifndef G_OS_WIN32 > + return strcmp (a, b); > > Why change g_strcmp0 to strcmp? You can see part of the answer in the diff context there. The function does: if (a == NULL) a = ""; if (b == NULL) b = ""; #ifndef G_OS_WIN32 return g_strcmp0 (a, b); ... (In reply to comment #21) > Review of attachment 228719 [details] [review]: > > Seems like this one could use a test? Already got one; this patch is needed to make gio/tests/g-icon pass. I'll add that to the commit message.
Created attachment 229071 [details] [review] win32: add gwin32compat.h, for utf8-renaming compatibility defines To avoid -Wmissing-prototype warnings, we need to prototype both the original and the _utf8 versions of all of the functions that have had _utf8-renaming on Windows. But duplicating all the prototypes is ugly, so rather than doing them "in-place", move them all to a new header file just for that. ==== Alternate version of the binary-compatibility-prototyping patch, which moves all the prototypes into their own header so they don't dirty up the rest of the headers. (At least for glib/; gmodule/gmodule.h still gets dirty.)
Review of attachment 229071 [details] [review]: I like this a *lot* better! There's just one thing I noticed - we're losing the #ifndef __GTK_DOC_IGNORE__ bit...or are we just going to avoid including gwin32compat.h in the doc sources list somehow?
Review of attachment 228720 [details] [review]: Sounds right...
Review of attachment 228721 [details] [review]: If only we had GSubprocess...
Review of attachment 228722 [details] [review]: One question: ::: glib/tests/test-printf.c @@ +726,3 @@ + */ +#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6) +_Pragma ("GCC diagnostic push") I've always spelled it #pramga GCC diagnostic push It looks like _Pramga is C99. Dunno whether it matters.
Review of attachment 228723 [details] [review]: Ok.
Review of attachment 228724 [details] [review]: Ugh...long double is a really screwed up type.
(In reply to comment #24) > There's just one thing I noticed - we're losing the #ifndef __GTK_DOC_IGNORE__ > bit...or are we just going to avoid including gwin32compat.h in the doc sources > list somehow? yes, that's what the change to docs/reference/glib/Makefile.am does; it's adding it to "IGNORE_HFILES" (In reply to comment #27) > +#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6) > +_Pragma ("GCC diagnostic push") > > I've always spelled it #pramga GCC diagnostic push > > It looks like _Pramga is C99. Dunno whether it matters. I just copied it from G_GNUC_BEGIN_IGNORE_DEPRECATIONS (which uses _Pragma because you can put that in a macro). The gcc version check there ensures that gcc has both _Pragma and "diagnostic push" though.
Ok, sounds good; everything in the "reviewed" state is "accepted-commit-with-or-without-changes-from-comments".
Attachment 228714 [details] pushed as 3ac6cfa - win32: prototype _glib_get_dll_directory() and _glib_get_locale_dir() Attachment 228715 [details] pushed as 731b469 - win32: define _WIN32_WINNT globally Attachment 228716 [details] pushed as f248c86 - win32: move some code into #ifdef G_OS_UNIX Attachment 228717 [details] pushed as b8c13a0 - win32: misc warning fixes Attachment 228718 [details] pushed as f80d8f1 - win32: various fixes to test programs Attachment 228719 [details] pushed as 468a166 - GLocalFile: canonicalize the initial directory separator Attachment 228720 [details] pushed as 75d2c18 - gvariant-internal.h: fix the include hack Attachment 228721 [details] pushed as d9e8fea - win32: make gio/tests/gdbus-proxy.c compile Attachment 228722 [details] pushed as fc3acd8 - win32: avoid printf format warnings on nonstandard formats Attachment 228723 [details] pushed as aa1418c - win32: work around broken winsock prototypes Attachment 228724 [details] pushed as 2628dc2 - gtestutils: don't try to print long doubles Attachment 229071 [details] pushed as 08f4f92 - win32: add gwin32compat.h, for utf8-renaming compatibility defines
Created attachment 229104 [details] [review] win32: re-fix the _utf8 compat function situation The previous fix didn't work, because every place within glib that used any of the functions also needed to be including win32compat.h. So, move the prototypes back to their original headers (but at least all in one place at the bottom).
not totally sure what happened here... I think I must have accidentally re-autogen'ed without --host at some point and so when I tested after redoing the _utf8 compat patch, I was accidentally testing the linux code, not win32...
Review of attachment 229104 [details] [review]: Ah =/ Hrm. Yeah...I guess this is the best then.
committed