Created attachment 371418 [details] [review]
tests/autoptr: Don't use /dev/null under Windows

A small fix for starters :)

Review of attachment 371418 [details] [review]:

Sure

A few of the tests are failing because mingw by default uses the old ~c89 printf from msvcrt.dll and the included gnulib isn't configured to cover the differences. In MSYS2 glib is patched to use __USE_MINGW_ANSI_STDIO which switches the underlying implementation with a C99 compatible one.

Two options:

1) Extend gnulib checks and include more of gnulib to be more C99 compatible (I guess, but I haven't tried that)
2) Use __USE_MINGW_ANSI_STDIO by default under mingw so that gnulib isn't needed (and expose it through pkg-config as well)

Wait, what? __USE_MINGW_ANSI_STDIO in pkg-config? What for?

(In reply to LRN from comment #4)
> Wait, what? __USE_MINGW_ANSI_STDIO in pkg-config? What for?

G_GNUC_PRINTF needs to match the the printf implementation/format constants and use __MINGW_PRINTF_FORMAT, which expands to different things depending on whether __USE_MINGW_ANSI_STDIO is defined and is included by the user.

I guess we could set G_GNUC_PRINTF at build time instead?

Here is the MSYS2 downstream patch btw: https://github.com/Alexpux/MINGW-packages/blob/master/mingw-w64-glib2/0017-glib-use-gnu-print-scanf.patch

__USE_MINGW_ANSI_STDIO affects *client* code, and G_GUNC_PRINTF affects the *glib* code. Therefore G_GNUC_PRINTF needs to match the code that is compiled in *glib*:

G_GNUC_SCANF doesn't affect glib at all (it's a macro purely for the clients to use) and thus provides no guarantees about implementation support.

G_GNUC_FORMAT is used only by glib gettext support, which relies on GNU gettext, which probably uses gnulib as well, so G_GNUC_FORMAT should match that.

G_GNUC_PRINTF should match gnulib printf implementation for g_print, g_strdup_printf, g_snprintf and many other functions that use G_GNUC_PRINTF and rely on internal gnulib implementation
AND
it should match msvc (ms_printf) or mingw_ansi_stdio (gnu_printf) implementation for all functions that do *not* rely on internal gnulib implementation.

Bug 725470 was dedicated to rectifying that.

(note that the above is focused on public glib APIs; G_GNUC_FORMAT is also used internally in many places).

Actually, now that i've reviewed the situation again, this makes the issue clearer, IMO:

1) G_GNUC_PRINTF *must* use "gnu_printf", because the implementation is always either glibc (when not on Windows) or gnulib (when on Windows).

2) We must somehow verify that all functions that use G_GNUC_PRINTF do indeed use gnulib implementation on Windows. Where they don't, we must remove G_GNUC_PRINTF attribute, or make them use gnulib implementation directly or indirectly.

3) It is an error for clients to use G_GNUC_PRINTF on a function that doesn't accept C99/GNU formats.

4) G_GNUC_SCANF can be anything (as i've said, it affects nothing in glib), though for symmetry it should probably be "gnu_scanf" as well. It does have "GNU" in its name after all.

5) It is an error for clients to use G_GNUC_SCANF on a function that doesn't accept C99/GNU formats.

6) G_GNUC_FORMAT *must* be "gnu_printf", as gettext also uses gnulib

7) We must somehow verify that all functions that use G_GNUC_FORMAT internally do indeed use gnulib implementation on Windows. Where they don't, we must remove the G_GNUC_FORMAT attribute, or make them use gnulib implementation directly or indirectly.

8) It is an error for clients to use G_GNUC_* attribute on their own function if it is not backed by an implementation that either uses G_GNUC_* itself (if they pass strings to glib functions that use G_GNUC_*) or an implementation that is known to support GNU/C99 formats.

9) It is an error for clients to pass GNU/C99 formats to functions that do not support GNU/C99 formats.

10) For convenience, glib might want to expand the G_*_FORMAT and/or G_*_MODIFIER macros with other variants:

G_GINT16_MS_MODIFIER
G_GINT16_MS_FORMAT
G_GUINT16_MS_FORMAT
G_GINT32_MS_MODIFIER
G_GINT32_MS_FORMAT
G_GUINT32_MS_FORMAT
G_GINT64_MS_MODIFIER
G_GINT64_MS_FORMAT
G_GUINT64_MS_FORMAT
G_GSIZE_MS_MODIFIER
G_GSSIZE_MS_MODIFIER
G_GSIZE_MS_FORMAT
G_GSSIZE_MS_FORMAT
G_GOFFSET_MS_MODIFIER
G_GOFFSET_MS_FORMAT
G_GINTPTR_MS_MODIFIER
G_GINTPTR_MS_FORMAT
G_GUINTPTR_MS_FORMAT

G_GINT16_GNU_MODIFIER
G_GINT16_GNU_FORMAT
G_GUINT16_GNU_FORMAT
G_GINT32_GNU_MODIFIER
G_GINT32_GNU_FORMAT
G_GUINT32_GNU_FORMAT
G_GINT64_GNU_MODIFIER
G_GINT64_GNU_FORMAT
G_GUINT64_GNU_FORMAT
G_GSIZE_GNU_MODIFIER
G_GSSIZE_GNU_MODIFIER
G_GSIZE_GNU_FORMAT
G_GSSIZE_GNU_FORMAT
G_GOFFSET_GNU_MODIFIER
G_GOFFSET_GNU_FORMAT
G_GINTPTR_GNU_MODIFIER
G_GINTPTR_GNU_FORMAT
G_GUINTPTR_GNU_FORMAT

which would be set to the correct strings for the given glib types to be used in MS and GNU implementations. The plain G_*_FORMAT and G_*_MODIFIER (without _GNU or _MS parts in the name) would be platform dependent (they would use MS formats on Windows and GNU formats everywhere else), with the assumption that they would be used by the clients with C runtime functions.

I do realize that this shifts significant portions of the burden to the clients, but we really have no control over which implementations they use in addition to glib. All we can do is to tell them the format specifiers for glib-specific types (so it becomes a choice between G_GUINT64_GNU_FORMAT and G_GUINT64_MS_FORMAT, not between %llu or %I64 or %lu or whatever).

Clients might have been thinking that G_*_FORMAT macros are usable with any print*()- or scan*()-like function. That wasn't true, and still isn't, and everyone will be better off if we make that clear. Same goes for G_GNUC_PRINTF. I guess we could also introduce G_GNUC_GNU_PRINTF and G_GNUC_MS_PRINTF... Is this what you meant by __MINGW_PRINTF_FORMAT?

We certainly can't (and shouldn't) force clients to __USE_MINGW_ANSI_STDIO (especially since that's not an option for, say, MSVC users), it's a choice for either the client code or the people who build it, not for us.

Thanks for the summary :)

Enforcing gnu_printf makes sense.

Regarding glib format constants: Can't we document that on Windows they are only valid to use for glib API? I think adding more constants just adds more confusion there. Also the MS constants would depend on which runtime you use if I'm not mistaken.

> Clients might have been thinking that G_*_FORMAT macros are usable with any print*()- or scan*()-like function. That wasn't true, and still isn't, and everyone will be better off if we make that clear. Same goes for G_GNUC_PRINTF. I guess we could also introduce G_GNUC_GNU_PRINTF and G_GNUC_MS_PRINTF... Is this what you meant by __MINGW_PRINTF_FORMAT?

No, __MINGW_PRINTF_FORMAT expands to ms_printf or gnu_printf under MinGW depending on __USE_MINGW_ANSI_STDIO. printf/__printf__ is always equal to ms_printf under MinGW afaik, thus the extra macro. Ignore it.

> We certainly can't (and shouldn't) force clients to __USE_MINGW_ANSI_STDIO (especially since that's not an option for, say, MSVC users), it's a choice for either the client code or the people who build it, not for us.

ok, makes sense.

Created attachment 371728 [details] [review]
docs: Add a note that the printf format macros might not  be compatible with system printf()

The current docs implied, by using the printf name, that the macros would
be compatible with printf(), but that's not always the case.

On Windows we use gnulib if the system printf isn't good enough.
This can happen on MinGW without __USE_MINGW_ANSI_STDIO set or with MSVC
with a varying degree of incompatibility.

Created attachment 371729 [details] [review]
Always assume that we use a gnu/c99 printf implementation

On Windows we use gnulib and elsewhere we use glibc or similar

Also change G_GNUC_PRINTF to use gnu_printf instead of __format__ if
possible because __format__ evaluates to ms_printf under MinGW,
but we use gnulib there and not the system printf.

Created attachment 371730 [details] [review]
meson: Don't use gnulib if __USE_MINGW_ANSI_STDIO is set under MinGW

If glib is build with __USE_MINGW_ANSI_STDIO we can assume a proper
printf implementation and we don't have to use the internal gnulib.

While our build tests should catch this case gnulib is currently forced
on Windows. Don't force it in case __USE_MINGW_ANSI_STDIO is set.

(In reply to Christoph Reiter (lazka) from comment #8)
> Thanks for the summary :)
> 
> Enforcing gnu_printf makes sense.
> 
> Regarding glib format constants: Can't we document that on Windows they are
> only valid to use for glib API? I think adding more constants just adds more
> confusion there. Also the MS constants would depend on which runtime you use
> if I'm not mistaken.

That might be sufficient.


(In reply to Christoph Reiter (lazka) from comment #9)
> Created attachment 371728 [details] [review] [review]
> docs: Add a note that the printf format macros might not  be compatible with
> system printf()
> 

Looks OK.

(In reply to Christoph Reiter (lazka) from comment #10)
> Created attachment 371729 [details] [review] [review]
> Always assume that we use a gnu/c99 printf implementation
> 

Also looks OK, but i'll need to make a build to see that it works as intended.

(In reply to Christoph Reiter (lazka) from comment #11)
> Created attachment 371730 [details] [review] [review]
> meson: Don't use gnulib if __USE_MINGW_ANSI_STDIO is set under MinGW
> 
> If glib is build with __USE_MINGW_ANSI_STDIO we can assume a proper
> printf implementation and we don't have to use the internal gnulib.
> 
> While our build tests should catch this case gnulib is currently forced
> on Windows. Don't force it in case __USE_MINGW_ANSI_STDIO is set.

I'm not sure about this. Originally the use of gnulib was due to the fact that printf implementation (MSVC back in the day, i guess?) was deemed insufficient for some reason. I wasn't around at the time. Either way, when i later asked tml about it, he said that he doesn't remember the reason why they used gnulib, and why they didn't use MinGW ANSI STDIO (if it even existed at the time).

Either way, i don't see any value in using MinGW ANSI STDIO over gnulib in this particular case - we already need gnulib to support MSVC anyway (unless you want to drop MSVC support, in which case i'm all for it!), so it would be better for all Windows builds to use the same code.

(In reply to LRN from comment #12)
> (In reply to Christoph Reiter (lazka) from comment #10)
> > Created attachment 371729 [details] [review] [review] [review]
> > Always assume that we use a gnu/c99 printf implementation
> > 
> 
> Also looks OK, but i'll need to make a build to see that it works as
> intended.

I found some problems (and a typo), so please wait for the next version :P

> (In reply to Christoph Reiter (lazka) from comment #11)
> > Created attachment 371730 [details] [review] [review] [review]
> > meson: Don't use gnulib if __USE_MINGW_ANSI_STDIO is set under MinGW
> > 
> > If glib is build with __USE_MINGW_ANSI_STDIO we can assume a proper
> > printf implementation and we don't have to use the internal gnulib.
> > 
> > While our build tests should catch this case gnulib is currently forced
> > on Windows. Don't force it in case __USE_MINGW_ANSI_STDIO is set.
> 
> I'm not sure about this. Originally the use of gnulib was due to the fact
> that printf implementation (MSVC back in the day, i guess?) was deemed
> insufficient for some reason. I wasn't around at the time. Either way, when
> i later asked tml about it, he said that he doesn't remember the reason why
> they used gnulib, and why they didn't use MinGW ANSI STDIO (if it even
> existed at the time).
> 
> Either way, i don't see any value in using MinGW ANSI STDIO over gnulib in
> this particular case - we already need gnulib to support MSVC anyway (unless
> you want to drop MSVC support, in which case i'm all for it!), so it would
> be better for all Windows builds to use the same code.

The current gnulib in glib isn't sufficient to fix the shortcomings in the old msvc runtime mingw uses by default, for example it still fails at printing inf/nan.
This would allow us to use __USE_MINGW_ANSI_STDIO on the gitlab CI for now.

Created attachment 371737 [details] [review]
Always assume that we use a gnu/c99 printf implementation

fixed a thinko on the gcc version check

(In reply to Christoph Reiter (lazka) from comment #13)
> (In reply to LRN from comment #12)
> > (In reply to Christoph Reiter (lazka) from comment #11)
> > > Created attachment 371730 [details] [review] [review] [review] [review]
> > > meson: Don't use gnulib if __USE_MINGW_ANSI_STDIO is set under MinGW
> > > 
> > > If glib is build with __USE_MINGW_ANSI_STDIO we can assume a proper
> > > printf implementation and we don't have to use the internal gnulib.
> > > 
> > > While our build tests should catch this case gnulib is currently forced
> > > on Windows. Don't force it in case __USE_MINGW_ANSI_STDIO is set.
> > 
> > I'm not sure about this.
> > i don't see any value in using MinGW ANSI STDIO over gnulib in
> > this particular case - we already need gnulib to support MSVC anyway (unless
> > you want to drop MSVC support, in which case i'm all for it!), so it would
> > be better for all Windows builds to use the same code.
> 
> The current gnulib in glib isn't sufficient to fix the shortcomings in the
> old msvc runtime mingw uses by default, for example it still fails at
> printing inf/nan.

Maybe an update from upstream gnulib is in order? I would have expected them to fix this. Or did you mean that there are shortcomings in the parts where we are *not* using gnulib? If that's the case, and if MSVC, using newer runtime, doesn't suffer from this, then MinGW ANSI STDIO would be appropriate.

Anyway, i'll try to test the patch and get back to you on that.

(In reply to LRN from comment #15)
> (In reply to Christoph Reiter (lazka) from comment #13)
> > (In reply to LRN from comment #12)
> > > (In reply to Christoph Reiter (lazka) from comment #11)
> > > > Created attachment 371730 [details] [review] [review] [review] [review] [review]
> > > > meson: Don't use gnulib if __USE_MINGW_ANSI_STDIO is set under MinGW
> > > > 
> > > > If glib is build with __USE_MINGW_ANSI_STDIO we can assume a proper
> > > > printf implementation and we don't have to use the internal gnulib.
> > > > 
> > > > While our build tests should catch this case gnulib is currently forced
> > > > on Windows. Don't force it in case __USE_MINGW_ANSI_STDIO is set.
> > > 
> > > I'm not sure about this.
> > > i don't see any value in using MinGW ANSI STDIO over gnulib in
> > > this particular case - we already need gnulib to support MSVC anyway (unless
> > > you want to drop MSVC support, in which case i'm all for it!), so it would
> > > be better for all Windows builds to use the same code.
> > 
> > The current gnulib in glib isn't sufficient to fix the shortcomings in the
> > old msvc runtime mingw uses by default, for example it still fails at
> > printing inf/nan.
> 
> Maybe an update from upstream gnulib is in order? I would have expected them
> to fix this. Or did you mean that there are shortcomings in the parts where
> we are *not* using gnulib? If that's the case, and if MSVC, using newer
> runtime, doesn't suffer from this, then MinGW ANSI STDIO would be
> appropriate.

Yeah, is has various checks for NEED_PRINTF_INFINITE_DOUBLE etc, and if you define those it tries to include more files not part of the gnulib in glib.
Upstream gnulib does lots of checks on the system sprintf to decide which parts to enable/include, e.g.: https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=m4/printf.m4;hb=7cdcfaad536a02b90be6c2104ac4a38bf620bbd1#l194

Then we need to replicate some of these tests (i mean, it might be possible to just decide which parts of the code are needed, and hardcode these decisions, but you never know...).

Review of attachment 371737 [details] [review]:

Should be fine, compiling GTK with this patch does not produce any new warnings or errors, when compared to compiling GTK with patches from bug 725470, which i've been using for quite some time.

(In reply to LRN from comment #17)
> Then we need to replicate some of these tests (i mean, it might be possible
> to just decide which parts of the code are needed, and hardcode these
> decisions, but you never know...).

I've tried to tinker with the gnulib *printf* code, to enable support for printing NaN correctly (and for some other things, while i'm at it), and that turned out to be somewhat more difficult than i though. Doable, but it will take a while for me to formulate a working patch.

Review of attachment 371728 [details] [review]:

::: glib/docs.c
@@ +56,3 @@
+ * glib might use a different printf implementation internally. They will
+ * always work with glib provided API like g_print() unless specified
+ * otherwise and any C99 compatible printf implementation.

I think this needs a bit of proofreading. How about:

Note that depending on the platform and build configuration, the format
macros might not be compatible with the system provided printf() function,
because GLib might use a different printf() implementation internally.
The format macros will always work with GLib API (like g_print()), and with
any C99 compatible printf() implementation.

Review of attachment 371730 [details] [review]:

::: meson.build
@@ +713,3 @@
+# (except exponent digit count https://sourceforge.net/p/mingw-w64/bugs/732/)
+if host_system == 'windows'
+  use_mingw_stdio = cc.get_define('__USE_MINGW_ANSI_STDIO') != '0'

If __USE_MINGW_ANSI_STDIO is not defined, get_define() will return an empty string for it, which (I think) will cause use_mingw_stdio to be set to TRUE. Is that what you intended?

@@ +716,3 @@
+else
+  use_mingw_stdio = false
+endif

Note: It might also be simpler to eliminate the if-statement:

use_mingw_stdio = (host_system == 'windows' and cc.get_define(…) …)

Review of attachment 371730 [details] [review]:

Also, the same changes will need to be made to configure.ac. We need to keep feature parity between the two build systems until we deprecate autotools.

Comment on attachment 371730 [details] [review]
meson: Don't use gnulib if __USE_MINGW_ANSI_STDIO is set under MinGW

This was doubly wrong, get_define() doesn't even pick up defines set through CFLAGS.

Created attachment 371917 [details] [review]
glib: update internal gnulib from upstream

Here, i've updated gnulib from upstream and
enabled a lot of code in it. Theoretically, it should
correctly print %e and NaN.

If you want a separate patch that shows the differences
between upstream gnulib source code and this code,
i can provide it separately.

That said, some tests still fail. For example, i don't think
that g_strerror() test will ever pass in its current form.

Review of attachment 371917 [details] [review]:

Some source files are lgpl 3+, that cannot be added to glib

(In reply to LRN from comment #24)
> Created attachment 371917 [details] [review] [review]
> glib: update internal gnulib from upstream
> 
> Here, i've updated gnulib from upstream and
> enabled a lot of code in it. Theoretically, it should
> correctly print %e and NaN.

The inf/nan tests work here, %e not. I noticed a few new meson warnings (re undef HAVE_ACOSF etc)

(In reply to Ignacio Casal Quinteiro (nacho) from comment #25)
> Review of attachment 371917 [details] [review] [review]:
> 
> Some source files are lgpl 3+, that cannot be added to glib

:(

Created attachment 371918 [details] [review]
docs: Add a note that the printf format macros might not  be compatible with system printf()

using pwithnall's wording

Created attachment 371919 [details] [review]
meson: Don't skip snprintf/vsnprintf checks under MinGW

The comment stated that the test isn't good enough, but it correctly
detects a C99 printf when I build with -D__USE_MINGW_ANSI_STDIO=1
and an incompatible printf without it.

Using mingw-w64 from current MSYS2.

(In reply to Ignacio Casal Quinteiro (nacho) from comment #25)
> Review of attachment 371917 [details] [review] [review]:
> 
> Some source files are lgpl 3+, that cannot be added to glib

This seems to be a limitation of gnulib-tool. The license comments actually state the these files are under GPLv3+, but gnulib-tool substitutes that for LGPLv3+ when invoked with --lgpl, since all these modules are under LGPL.

The limitation here is the fact that all modules that i've imported are under LGPLv2+ (maybe some are LGPLv2, but none are LGPLv3 or LGPLv3+), but gnulib-tool is not smart enough to fix the version of the license too.

There are three ways to fix this:
1) Fix gnulib-tool

2) Manually fix the licenses

3) Keep the licenses as-is, but add a comment to README about the actual license for the code; alternatively, add a COPYING file with the same contents.

(1) might be difficult, if doable, and i have no desire to touch that - we are unlikely to need this functionality often
(2) is doable, but it makes the gnulib-glib diff larger, which is something i'm trying to avoid
(3) is even simpler, but i don't know how well that would work, from legal point of view.

(In reply to Christoph Reiter (lazka) from comment #26)
> (In reply to LRN from comment #24)
> > Created attachment 371917 [details] [review] [review] [review]
> > glib: update internal gnulib from upstream
> > 
> > Here, i've updated gnulib from upstream and
> > enabled a lot of code in it. Theoretically, it should
> > correctly print %e and NaN.
> 
> The inf/nan tests work here, %e not. 

Which tests exactly?

>I noticed a few new meson warnings (re
> undef HAVE_ACOSF etc)
> 

Yeah, the meson part of the patch is not very clean. And the prospect of adding autotools support for this new code fills me with dread.

Created attachment 371920 [details] [review]
tests/strfuncs: handle unknown error codes when testing  g_strerror

The tests checks that g_strerror returns unique error messages for
all error codes between 1-200, but under Windows only a small range of them
is actually used: https://msdn.microsoft.com/en-us/library/t3ayayh1.aspx

Change the test to check that the returned message is either unique or
matches the error message for unknown codes instead.

Created attachment 371921 [details] [review]
tests/strfuncs: mingw-w64 prints 3 digits for the %e  exponent

Filed and fixed upstream: https://sourceforge.net/p/mingw-w64/bugs/732/

Once we get a new release in MSYS2 or when we get better gnulib integration
this special case needs to be removed again, but for now this will do.

Created attachment 371922 [details] [review]
win32: make g_cond_wait_until() wait at least until  end_time before returning with a timeout

The tests in test_async_queue_timed() assume that g_async_queue_timeout_pop()
and in turn g_cond_wait_until() wait at least until end_time
before returning, i.e. calling g_get_monotonic_time() after the timeout should result
in a value equal or larger than the timeout end time.

For the win32 implementation of g_cond_wait_until() this isn't the case which
makes those tests fail.

There are three reasons why the function returns early:

1) The underlying API works with milliseconds and the timeout gets rounded
   down, resulting in a too small timeout value.
2) In case the timeout is too large to be passed to the API it gets limited
   (there is also a bug because it converts INFINITE to milliseconds while
   they already are, but using INFINITE would be wrong as well, as passing
   a large timeout is not the same as blocking forever really)
3) Even with the rounding changed the underlying API still returns a bit early
   sometimes on my machine (relative to g_get_monotonic_time())

This changes the implementation to round up to the next millisecond (fixing 1)
and to wait again in case a timeout occurs but the end time hasn't been
reached yet (fixing 2 and 3).

This makes the test_async_queue_timed() tests pass.

Created attachment 371923 [details] [review]
tests: Disable positional parameter printf tests with the  mingw-w64 printf implementation

Positional parameters are a POSIX extension and not supported by the mingw-w64
printf implementation.

Created attachment 371924 [details] [review]
tests/logging: Don't hardcode the result of logging a  pointer

The output of the %p type is implementation defined and on Windows we get
leading zeros depending on the pointer type size. Instead of adding
ifdeffery use g_snprintf() to generate the expected message.

Created attachment 371925 [details] [review]
g_usleep: round up the next millisecond on Windows

The timer tests expect that a small value for sleep does not result in
no sleep at all. Round up to the next millisecond to bring it more in line
with other platforms.

This fixes the glib/timer tests.

(In reply to LRN from comment #29)
> (In reply to Ignacio Casal Quinteiro (nacho) from comment #25)
> > Review of attachment 371917 [details] [review] [review] [review]:
> > 
> > Some source files are lgpl 3+, that cannot be added to glib
> 
> This seems to be a limitation of gnulib-tool. The license comments actually
> state the these files are under GPLv3+, but gnulib-tool substitutes that for
> LGPLv3+ when invoked with --lgpl, since all these modules are under LGPL.
> 
> The limitation here is the fact that all modules that i've imported are
> under LGPLv2+ (maybe some are LGPLv2, but none are LGPLv3 or LGPLv3+), but
> gnulib-tool is not smart enough to fix the version of the license too.

Well, I guess that if they decided to use lgpl3 it means that adding lgpl3 is on purpose so it might be they want that code licensed that way.
I would choose 2 if you get an statement (mail, whatever) from the maintainer saying that the code is lgpl2 or lgpl2+

(In reply to Christoph Reiter (lazka) from comment #31)
> Created attachment 371921 [details] [review] [review]
> tests/strfuncs: mingw-w64 prints 3 digits for the %e  exponent
> 
> Filed and fixed upstream: https://sourceforge.net/p/mingw-w64/bugs/732/
> 
> Once we get a new release in MSYS2 or when we get better gnulib integration
> this special case needs to be removed again, but for now this will do.

This is deliberate in gnulib code (grep for "Produce the same number of exponent digits"). I've fixed that in my patch, though i'm not sure why gnulib maintainers decided to do this.

(In reply to LRN from comment #29)
> > The inf/nan tests work here, %e not. 
> 
> Which tests exactly?

Ignore that, I was testing it on top of my patches where I had changed the expected result.

(In reply to Ignacio Casal Quinteiro (nacho) from comment #36)
> (In reply to LRN from comment #29)
> > (In reply to Ignacio Casal Quinteiro (nacho) from comment #25)
> > > Review of attachment 371917 [details] [review] [review] [review] [review]:
> > > 
> > > Some source files are lgpl 3+, that cannot be added to glib
> > 
> > This seems to be a limitation of gnulib-tool. The license comments actually
> > state the these files are under GPLv3+, but gnulib-tool substitutes that for
> > LGPLv3+ when invoked with --lgpl, since all these modules are under LGPL.
> > 
> > The limitation here is the fact that all modules that i've imported are
> > under LGPLv2+ (maybe some are LGPLv2, but none are LGPLv3 or LGPLv3+), but
> > gnulib-tool is not smart enough to fix the version of the license too.
> 
> Well, I guess that if they decided to use lgpl3 it means that adding lgpl3
> is on purpose so it might be they want that code licensed that way.
> I would choose 2 if you get an statement (mail, whatever) from the
> maintainer saying that the code is lgpl2 or lgpl2+

Does https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=COPYING;h=6ad3d4b51ca0f52023f7bf829195b9d6d0e71dfe;hb=HEAD and the fact that none of the module files in 'modules' subdir have LGPLv3 in them (there are LPGLv3 modules in uni* subdirs, but we don't use these; also, bzero and unistring are LGPLv3, but we don't use these either) satisfy your need for "an statement"?

(In reply to LRN from comment #29)
> Yeah, the meson part of the patch is not very clean. And the prospect of
> adding autotools support for this new code fills me with dread.

I wouldn't mind dropping mingw support from the autotools build.

Created attachment 371930 [details] [review]
autotools: drop support for MinGW, recommend meson instead.

Discuss! :D

Review of attachment 371918 [details] [review]:

++

Review of attachment 371919 [details] [review]:

OK.

Review of attachment 371920 [details] [review]:

::: glib/tests/strfuncs.c
@@ +1336,3 @@
   setlocale (LC_ALL, "C");
 
+  unknown_str = g_strdup (g_strerror (-1));

No need to g_strdup() this, as g_strerror() says the string pointer is guaranteed to remain valid for the lifetime of the process.

Might be useful to add a comment briefly explaining why we might expect some unknown strings. For example, you could stick this just above the `static void\ntest_strerror (void)`:

/* Test the strings returned by g_strerror() are valid and unique. On Windows, fewer than 200 error numbers are used, so we expect some strings to return a generic ‘unknown error code’ message. */

@@ +1352,2 @@
   g_hash_table_iter_init (&iter, strs);
   while (g_hash_table_iter_next (&iter, (gpointer *)&str, NULL))

While you’re here, it looks to me like this while-loop is entirely redundant, since it checks all the strings in the hash table for UTF-8 validity — but we check each string for UTF-8 validity before adding them to the hash table in the first place. Fancy removing this as a follow-up commit?

Review of attachment 371921 [details] [review]:

::: glib/tests/strfuncs.c
@@ +1039,3 @@
   check_strtod_number (-0.75, "%5.2f", "-0.75");
+#if defined(_MSC_VER) || defined(__MINGW32__)
+  /* See https://sourceforge.net/p/mingw-w64/bugs/732/ for mingw-w64 */

Please change this comment to “FIXME: See https://…” so that in future we can `git grep FIXME` and tidy up hacks like this.

Created attachment 372293 [details] [review]
tests/strfuncs: handle unknown error codes when testing  g_strerror

Created attachment 372294 [details] [review]
tests/strfuncs: mingw-w64 prints 3 digits for the %e  exponent

Created attachment 372295 [details] [review]
tests/strfuncs: drop some redundant test code

The loop was testing that all strings in the hash table are valid
utf-8, but the loop filling the hash table is already doing that.

Review of attachment 372293 [details] [review]:

++, thanks

Review of attachment 372294 [details] [review]:

++

Review of attachment 372295 [details] [review]:

++

Review of attachment 371924 [details] [review]:

Please push with this change, thanks.

::: glib/tests/logging.c
@@ +433,3 @@
     { "GLIB_DOMAIN", "some-domain", -1 },
     { "PRIORITY", "5", -1 },
+    { "MESSAGE", NULL, -1 },

Nitpick: Maybe use `"String assigned using g_snprintf() below"` rather than NULL, to make it a bit more obvious that the NULL is not intentional.

Review of attachment 371923 [details] [review]:

Not sure about this one. g_printf() explicitly documents that it supports positional parameters. If the native printf() implementation on mingw-w64 doesn’t support that, then we might have to use gnulib on mingw-w64.

::: glib/tests/markup-escape.c
@@ +10,3 @@
+/* mingw-w64 does not support the positional parameter extension */
+#undef SUPPORTS_POSITIONAL_PARAMETERS
+#endif

Why not

#if !defined(__MINGW32__) || !defined(__USE_MINGW_ANSI_STDIO) || ((__USE_MINGW_ANSI_STDIO + 0) == 0)
#define SUPPORTS_POSITIONAL_PARAMETERS
#endif

What is the `+ 0` for?

Review of attachment 371922 [details] [review]:

Nice analysis. I have one suggestion.

::: glib/gthread-win32.c
@@ +312,2 @@
+  start_time = g_get_monotonic_time ();
+  do

Might be clearer to s/success/signalled/ and write the loop as:

gboolean waited = FALSE;

for (start_time = g_get_monotonic_time (); waited && (signalled || start_time >= end_time); start_time = g_get_monotonic_time ())
  {
    …
    /* set waited = TRUE just before calling SleepConditionVariableSRW() */
    /* no need for the explicit `break` statement */
    …
  }

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/1371.