After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 688109 - win32 warning/error fixes
win32 warning/error fixes
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: win32
unspecified
Other All
: Normal normal
: ---
Assigned To: gtk-win32 maintainers
gtk-win32 maintainers
Depends on:
Blocks:
 
 
Reported: 2012-11-11 19:44 UTC by Dan Winship
Modified: 2012-11-16 13:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
win32: prototype the binary-compatibility functions (11.64 KB, patch)
2012-11-11 19:44 UTC, Dan Winship
reviewed Details | Review
win32: prototype _glib_get_dll_directory() and _glib_get_locale_dir() (4.78 KB, patch)
2012-11-11 19:44 UTC, Dan Winship
committed Details | Review
win32: define _WIN32_WINNT globally (3.20 KB, patch)
2012-11-11 19:44 UTC, Dan Winship
committed Details | Review
win32: move some code into #ifdef G_OS_UNIX (3.80 KB, patch)
2012-11-11 19:45 UTC, Dan Winship
committed Details | Review
win32: misc warning fixes (19.44 KB, patch)
2012-11-11 19:45 UTC, Dan Winship
committed Details | Review
win32: various fixes to test programs (7.54 KB, patch)
2012-11-11 19:45 UTC, Dan Winship
committed Details | Review
GLocalFile: canonicalize the initial directory separator (983 bytes, patch)
2012-11-11 19:45 UTC, Dan Winship
committed Details | Review
gvariant-internal.h: fix the include hack (1.46 KB, patch)
2012-11-11 19:45 UTC, Dan Winship
committed Details | Review
win32: make gio/tests/gdbus-proxy.c compile (1.04 KB, patch)
2012-11-11 19:45 UTC, Dan Winship
committed Details | Review
win32: avoid printf format warnings on nonstandard formats (2.27 KB, patch)
2012-11-11 19:45 UTC, Dan Winship
committed Details | Review
win32: work around broken winsock prototypes (1.94 KB, patch)
2012-11-11 19:45 UTC, Dan Winship
committed Details | Review
gtestutils: don't try to print long doubles (2.21 KB, patch)
2012-11-11 19:45 UTC, Dan Winship
committed Details | Review
win32: add gwin32compat.h, for utf8-renaming compatibility defines (18.09 KB, patch)
2012-11-15 17:55 UTC, Dan Winship
committed Details | Review
win32: re-fix the _utf8 compat function situation (21.24 KB, patch)
2012-11-15 23:21 UTC, Dan Winship
accepted-commit_now Details | Review

Description Dan Winship 2012-11-11 19:44:50 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.
Comment 1 Dan Winship 2012-11-11 19:44:53 UTC
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.
Comment 2 Dan Winship 2012-11-11 19:44:56 UTC
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.)
Comment 3 Dan Winship 2012-11-11 19:44:58 UTC
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.)
Comment 4 Dan Winship 2012-11-11 19:45:00 UTC
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.
Comment 5 Dan Winship 2012-11-11 19:45:02 UTC
Created attachment 228717 [details] [review]
win32: misc warning fixes
Comment 6 Dan Winship 2012-11-11 19:45:05 UTC
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.
Comment 7 Dan Winship 2012-11-11 19:45:08 UTC
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.
Comment 8 Dan Winship 2012-11-11 19:45:10 UTC
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.
Comment 9 Dan Winship 2012-11-11 19:45:12 UTC
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...
Comment 10 Dan Winship 2012-11-11 19:45:15 UTC
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.
Comment 11 Dan Winship 2012-11-11 19:45:18 UTC
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.)
Comment 12 Dan Winship 2012-11-11 19:45:20 UTC
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.
Comment 13 Dan Winship 2012-11-15 13:53:20 UTC
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.
Comment 14 Colin Walters 2012-11-15 14:55:46 UTC
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.
Comment 15 Colin Walters 2012-11-15 14:57:47 UTC
Review of attachment 228714 [details] [review]:

Looks right.
Comment 16 Colin Walters 2012-11-15 14:59:17 UTC
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.
Comment 17 Colin Walters 2012-11-15 15:00:53 UTC
Review of attachment 228716 [details] [review]:

Looks right.
Comment 18 Colin Walters 2012-11-15 15:11:35 UTC
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...
Comment 19 Colin Walters 2012-11-15 15:15:02 UTC
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?
Comment 20 Colin Walters 2012-11-15 15:15:03 UTC
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?
Comment 21 Colin Walters 2012-11-15 15:15:42 UTC
Review of attachment 228719 [details] [review]:

Seems like this one could use a test?
Comment 22 Dan Winship 2012-11-15 15:50:50 UTC
(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.
Comment 23 Dan Winship 2012-11-15 17:55:55 UTC
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.)
Comment 24 Colin Walters 2012-11-15 18:19:19 UTC
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?
Comment 25 Colin Walters 2012-11-15 18:20:47 UTC
Review of attachment 228720 [details] [review]:

Sounds right...
Comment 26 Colin Walters 2012-11-15 18:21:41 UTC
Review of attachment 228721 [details] [review]:

If only we had GSubprocess...
Comment 27 Colin Walters 2012-11-15 18:23:46 UTC
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.
Comment 28 Colin Walters 2012-11-15 18:24:30 UTC
Review of attachment 228723 [details] [review]:

Ok.
Comment 29 Colin Walters 2012-11-15 18:36:54 UTC
Review of attachment 228724 [details] [review]:

Ugh...long double is a really screwed up type.
Comment 30 Dan Winship 2012-11-15 18:38:56 UTC
(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.
Comment 31 Colin Walters 2012-11-15 18:53:15 UTC
Ok, sounds good; everything in the "reviewed" state is "accepted-commit-with-or-without-changes-from-comments".
Comment 32 Dan Winship 2012-11-15 19:36:52 UTC
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
Comment 33 Dan Winship 2012-11-15 23:21:47 UTC
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).
Comment 34 Dan Winship 2012-11-15 23:23:46 UTC
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...
Comment 35 Colin Walters 2012-11-16 01:46:37 UTC
Review of attachment 229104 [details] [review]:

Ah =/  Hrm.  Yeah...I guess this is the best then.
Comment 36 Dan Winship 2012-11-16 13:54:11 UTC
committed