Without protoypes, the output of the above isn't really
interesting; it would be more useful to write a test
program that showed what the results were at every stage.

I'm not exactly sure what you're looking for. Does this help?

50        d = pow (2.0, -1024.1);
(gdb) 
51        x = g_ascii_dtostr (buffer, sizeof (buffer), d);
(gdb) 
52        e = g_ascii_strtod (x, NULL);
(gdb) 
53        g_print("%s\n", x);
(gdb) 
5.190168296483618e-309
55        g_assert (d == e);
(gdb) p d
$1 = 5.1901682964836274e-309
(gdb) p x
$2 = (gchar *) 0xbfbfb9c4 "5.190168296483618e-309"
(gdb) p e
$3 = 5.1901682964836176e-309
(gdb)

I think the strtod test is just overoptimistic, should not use == to
compare two floats.

Actually, the C standard requires a perfect round trip here.

Then again, the implementation that we have in the included
Trio library almost certainly doesn't meet this requirement....

(Did GLib decide to use Trio as compiled?)

This seems to be trio's problem.
During configuration, the following test fail for NetBSD:
. checking whether printf supports positional parameters
  (which standard describes this?)
and thus trio gets used.
  

Positional parameters are part of the Unix and POSIX standards.

Thanks for the pointer. I filed a bug report with NetBSD to implement
positional parameters.

Owen, I believe that C99 only requires a perfect round trip here if
the used precision is less than DECIMAL_DIG, which may be as low as
10. But admittedly, the gcc which I use here sets DECIMAL_DIG to 21,
yet trio seems to deviate already at the 14th place.

  d = pow (2.0, -1024.1);
  printf ("%.17g (glibc)\n", d);  
  g_print ("%.17g (trio)\n", d);

5.1901682964836274e-309 (glibc)
5.1901682964836177e-309 (trio)

(using %.17g here since that is what g_ascii_dtostr () uses)

Note that DECIMAL_DIG isn't arbitrary, but is specified 
in terms of the number of binary digits in the widest
supported floating point type in such a way that conversion
from binary to decimal with DECIMAL_DIG digits and back
is exact.

The reason that DECIMAL_DIG is bigger than the value
that g_ascii_dtostr is using is because GCC on x86
supports a 80bit long double, so DECIMAL_DIG > DBL_DIG.

So, the C standard does require that round trip conversion
from double to decimal with DBL_DIG digits and back
is exact, what this assertion is asserting.

But DBL_DIG is only 15 here, which is less than the 17 used by
g_strtod(). 

I don't recall the exact reasoning behind the 17 .. it may
have just have been being conservative. But in any case.
the C standard rules are supposed to mean that when you
printf() and then read back with >= DBL_DIGITS, you
get an exact round trip even if the decimal result for 
> DBL_DIGITS isn't exact.

I believe the wording in C99 5.2.4.2.2 only implies that the results of
printing with DBL_DIG digits and printing-reading-printing will be
identical (ie the DBL_DIG digits won't change, the actual double value
may change), but trio fails even with this interpretation.

What about using %a instead of %.17g - produces a more compact result
and is specified to be precise. It's a C99 addition, but we require
specific printf features anyway. Unfortunately, the trio
implementation of it seems to be broken as well.

Since %a is broken in trio, I spent some time investigating how one
would go about implementing the functionality. I came up with three
versions of a print_double() function producing a %a-like format. 
The first two rely on the GDoubleIEEE574 union which glib provides,
thus will work only for IEEE doubles (this would reintroduce a
dependency on IEEE FP which we got rid of when we dropped the
homegrown print_string_upper_bound() implementation). The difference
between the first two functions is that one uses long long, which the
other one avoids (don't know if we generally avoid long long in glib).
The third function tries to achieve its goal in a generic way, relying
only on C99  functionality (signbit, frexp, DBL_MANT_DIG, FLT_RADIX
and fpclassify). Theoretically, this should work for non-IEEE FP
formats (untested). Limited testing seems to show that all three
functions give round-trip equality. 
I'd like to propose that we should replace g_ascii_dtostr by a
function like this.

Created attachment 13268 [details]
alternate double formatters

I don't really like the idea of changing dtostr to use %a;
I suspect people are using dtostr in contexts where readability
by things other than strtod (including humans) is a factor.

Putting on 2.4.0, since fixing the decimal conversion in
Trio is probably a major undertaking.

Another thing that's broken in trio (tested on Windows, with both gcc 
and MSVC), is %e with more than DBL_DIG precision. For instance 
g_printf("%0.40e", 14.) produces 
1.3999999999999999111821580299874767661095e+01 . (Note that 14 is 
exactly representable in floating point, so I don't see why it 
shouldn't produce 1.4000000000000000000000000000000000000000e+01).

When read by scanf("%e"), the 1.3999...e+01 above indeed produces 
something a tiny bit less than 14. 

This causes an interesting problem in GIMP, where 
doubles are transported from/to the plug-ins by g_snprintf'ing them 
with a %0.50e format and reading with scanf ("%e")...

I haven't yet checked whether using trio's scanf would help in 
getting the roundtrip correct, but I doubt it.
 

Changing OS to "All", trio problems aren't NetBSD specific.

Since we're using trio mainly in order to get C99 snprintf return
value semantics and positional parameters, maybe we could investigate
patching  
trio to use the system printf for floating point formatting. Don't
know how feasible that is, though.

Here is a quick implemenation of that idea, which makes strtod-test
pass  with trio. It obviously still has a problem with printing pi to
the first 1025 digits...

Created attachment 14397 [details] [review]
patch

Another idea might be to look into replacing trio by the LGPLed vasnprintf 
implemenation in gnulib (http://www.gnu.org/software/gnulib/) which seems to 
implement positional parameters and C99 snprintf semantics on top of the 
system
sprintf.

More accurate summary.

Here is a patch and tarball to replace the included trio printf
implementation by an implementation based on the vasnprintf found in
gnulib. This looks a lot cleaner than trio, passes strfunc-test and
strtod-test, and is probably more portable than trio (at least it uses
proper autoconfigury). I like it. What do you think, Owen ? 

Created attachment 14484 [details] [review]
patch

Created attachment 14485 [details]
additional sources

Forgot to mention: dumping all these macros into acinclude.m4 is
probably not the optimal way to handle them. If we use this, we should
probably patch the gnulib files to use g_malloc instead of malloc.

Hmmm, I can't say I have a definitive answer, but some thoughts:

 - The gnulib code is certainly closer to what we want -
   as compatible with the system scanf() as possible, 
   no incorrect math code, no strange extra extensions.

 - The fact that it only has vasprintf() means that there
   is going to be some slowdown for printf(), sprintf(),
   fprintf(), as compared to trio, since we have to
   allocate the buffer, then throw it away. 

   But vasprintf() is usually what we want (including
   for g_printe(), g_printerr when we do output conversion)
   so probably not worth worrying about.

 - Going this direction does allow less flexibility about
   systems with truly broken printf() implementations;
   still, if we wanted to go that direction, we'd really
   want to bring in our own floating-point formatting
   implementation, GNU MP, etc. Probably better to be
   lightweight.

 - Test cases definitely would be helpful in ensuring
   confidence in swapping out the code base.

A few random thoughts from reading the code:

 - I'd rather use _g_gnulib than _gnulib as the 
   namespace prefix. 

 - We need to #define all the exported symbols in
   the gnulib/ dir into a private namespace.

 - There is a FIXME about non-C99 snprintf()
   in the code which we can deal with using our
   configure check and falling back to the sprintf()
   case where a conservatively large buffer is
   allocated.

 - I'd probably use fwrite() rather than fputs()
   in _gnulib_vfprintf().

Created attachment 14910 [details] [review]
new patch

Created attachment 14911 [details]
new sources

The new patch tries to address most of the issues you raised, except
for a testsuite.

Created attachment 15050 [details]
test cases

Here are some tests for g_snprintf. Note that the test probably
folllow C99/SUS3 a bit too closely; e.g. they need the NAN and
INFINITY C99 macros which may not be present on many machines.

* printf.h doesn't handle errors.

* About the configure changes (actually an old problem):

 if test "x$ac_cv_sizeof_long_long" = "x8" &&
    test -z "$glib_cv_long_long_format" ; then
-  need_trio=yes
+  need_included_printf=yes
 fi

  Isn't this wrong on a system where both long and
  long long are 64-bit?

* Also, since the gnulib printf doesn't have code
  to handle long-long integers, perhaps this shouldn't 
  be in there at all. (This is actually a bit of
  a problem, since we are currently guaranteeing
  the ability to use long long integers with our
  printf functions.)

=== 

Looking at the tests, they look good. Some thoughts are:

 - They may be too strong to have in make check
   (that is, they test things that we don't
   rely on for correct functionality of GLib/GTK+,
   and probably don't work everywhere like %a, etc, etc.) 
   It strikes me that we need a smaller set to actually 
   run in make check to test stuff that "should work
   anywhere" and stuff that we rely upon.
 - Long double usage needs to be guarded (or
   omitted ... GLib has no long-double typedefs
   or other support.)

That should be "printf.c doesn't handle errors" if it wasn't
obvious.

Since configure already fails if there is no 64bit integer type, it
should probably fail as well if the system printf doesn't support that
type. Regarding error handling, I'm not quite sure what you want to
see here. I've attached a printf.c with some g_return_val_if_fail()s,
is that what you expected ?
I'll also attach a reduced set of printf tests, omitting a lot of the
more esoteric features (long long, long double, ptrdiff_t, intmax_t, 
%a, %g, etc) and adding the glib format definitions. 

Created attachment 15519 [details]
printf.c with error handling

Created attachment 15520 [details]
reduced set of tests

The question is - are there systems that we want to support
where the printf doesn't support the 64bit types.

For the type itself we can say "use GCC" for systems where
the native CC isn't up to 64 bit ints. But we don't have
that same option of printf. 

(We used to make 64-bit printf types optional - not guaranteed
to exist, but there was that guarantee in 2.2.)

For error handling in printf.c, I'm not talking about
g_return_if_fail(), but the case where printf() fails,
because the format string is unparseable or because
malloc fails.

On failure, _g_gnulib_vasnprintf returns NULL, and you
should propagate that as negative return from printf,
etc; currrently it looks like you'll just get a segfault
in most places.

Hmm, how hard would it be to implement basic %lld support for
defective systems ? 

Regarding the error handling, I see that SUS demands that printf et al
return < 0 on failure. I also note that we currently have

g_return_val_if_fail (..., 0)

throughout gprintf.c. So that should be changed to 

g_return_val_if_fail (..., -1) 

in addition to making sure the functions in printf.c return -1 if 
vasnprintf() fails for any reason.

Finally, I notice that the gnulib vasnprintf (like the glibc printf
functions) fail to fail on certain errors, e.g.

vsnprintf (buf, 5, "%4$d", "not valid") 

returns 11 and prints "-107" on my system.

Created attachment 15544 [details] [review]
corrected failure return values for gprintf.c

*** Bug 110684 has been marked as a duplicate of this bug. ***

Here is a version of vasnprintf.c which contains code to format long
long itself ifndef HAVE_LONG_LONG_FORMAT. The responsible function
print_long_long was adapted from corresponding trio code and should be
reasonable complete except for grouping, which I omitted because it
would introduce locale dependencies. We should probably warn if we meet
%'lld.

Remaining tasks: 
* Adjust configure in to define HAVE_LONG_LONG_FORMAT if system printf
understands %lld.
* Move print_long_long to a separate c file to shrink the vasnprintf.c
patch.
* Add some more %lld tests to printf-test.c above.

Created attachment 15721 [details]
vasnprintf.c with %lld support

Btw. I noticed that glib provides defines for gint64 analoga of %lld
and %llu, but not %llo, %llx or %llX. Is that intentional ?

Another problem with the guaranteed 64bit type support which I just 
noticed: in 2.2, we always define G_GINT64_FORMAT, and the 
documentation of the macro speaks about printing and scanning,


but we don't provide a g_sscanf() which is guaranteed to handle 


64bit ints.

* I think part of my comments above about configure got
  lost; the  problem with:
 
  if test "x$ac_cv_sizeof_long_long" = "x8" &&
     test -z "$glib_cv_long_long_format" ; then
     need_included_printf=yes
  fi

  is simply that if you have 64-bit longs, then you don't
  need a printf format for long_long.

* For the printf error handling, instead of:

  result = _g_gnulib_vasnprintf (NULL, &length, format, args);
  g_return_val_if_fail (result != NULL, 0);
 
  I'd just do:

  result = _g_gnulib_vasnprintf (NULL, &length, format, args);
  if (!result)
    return -1;

  on the principle that the _g_gnulib_* functions should work
  as identically to what they are emulating as possible, which
  doesn't include printing warnings to standard error on error
  conditions.  Plus g_return_val_if_fail() is supposed to 
  only be for programmatic errors, and vasnprintf() can
  return NULL on out-of-memory.

* In a fairly casual look, the long-long formatting stuff
  looks reasonable ... I didn't see any problems with it.

* It would be good to send the LONG_LONG emulation stuff
  back upstream once you're happy with it. (Probably obvious)

* I don't think it would hurt to add 64-bit oxX (please
  note that G_GINT64_FORMAT is *not* necessarily equivalent
  to ll; it might be equivalent to l), if there are 
  actually equivalents on all platforms. Looking at what
  we have currently, I think you could also define a
  G_GINT64_MODIFIER and do G_GINT64_MODIFIER "o"; since
  C99 has 'll', I don't think it's necesarry to worry
  that some other variant not in this pattern will show
  up.

* I wouldn't worry to much about the scanf issue, except for 
  perhaps fixing up the docs; parsing with scanf is frequently 
  a bit dubious anyways. We should probably fix up the 
  docs and tell people to use g_ascii_strtoull. (Might not
  hurt to add g_ascii_strtoll()...)

* I'm fine with you going ahead and replacing trio in HEAD
  with this code.

Committed after fixing up the error handling a bit and adding
G_GINT{16,32,64}_MODIFIER. I'll file a separate bug about
g_ascii_strtoll().

*** Bug 122347 has been marked as a duplicate of this bug. ***