On Wed, 24 Feb 2021, Jonathan Wakely wrote:
> On 23/02/21 11:30 -0500, Patrick Palka via Libstdc++ wrote:
> > On Mon, 22 Feb 2021, Patrick Palka wrote:
> >
> > > This makes the hexadecimal section of the long double std::to_chars
> > > testcase more robust by avoiding false-negative FAILs due to printf
> > > using a different leading hex digit than us, and by additionally
> > > verifying the correctness of the hexadecimal form via round-tripping
> > > through std::from_chars.
> > >
> > > Tested on x86, x86_64, powerpc64be, powerpc64le and aarch64. Does this
> > > look OK for trunk?
> >
> > The commit message could explain the issue better, so here's v2 with a
> > more detailed commit message.
> >
> > -- >8 --
> >
> > Subject: [PATCH] libstdc++: Robustify long double std::to_chars testcase
> > [PR98384]
> >
> > The long double std::to_chars testcase currently verifies the
> > correctness of its output by comparing it to that of printf, so if
> > there's a mismatch between to_chars and printf, the test FAILs. This
> > works well for the scientific, fixed and general formatting modes,
> > because the corresponding printf conversion specifiers (%e, %f and %g)
> > are rigidly specified.
> >
> > But this doesn't work so well for the hex formatting mode because the
> > corresponding printf conversion specifier %a is more flexibly specified.
> > For instance, the hexadecimal forms 0x1p+0, 0x2p-1, 0x4p-2 and 0x8p-3
> > are all equivalent and valid outputs of the %a specifier for the number
> > 1. The apparent freedom here is the choice of leading hex digit -- the
> > standard just requires that the leading hex digit is nonzero for
> > normalized numbers.
> >
> > Currently, our hexadecimal formatting implementation uses 0/1/2 as the
> > leading hex digit for floating point types that have an implicit leading
> > mantissa bit which in practice means all supported floating point types
> > except x86 long double. The latter type has a 64 bit mantissa with an
> > explicit leading mantissa bit, and for this type our implementation uses
> > the most significant four bits of the mantissa as leading hex digit.
> > This seems to be consistent with most printf implementations, but not
> > all, as PR98384 illustrates.
> >
> > In order to avoid false-positive FAILs due to arbitrary disagreement
> > between to_chars and printf about the choice of leading hex digit, this
> > patch makes the testcase's verification via printf conditional on the
> > leading hex digits first agreeing. An additional verification step is
> > also added: round-tripping the output of to_chars through from_chars
> > should yield the original value.
> >
> > Tested on x86, x86_64, powerpc64be, powerpc64le and aarch64. Does this
> > look OK for trunk?
>
> > @@ -50,6 +51,38 @@ namespace detail
> > void
> > test01()
> > {
> > + // Verifies correctness of the hexadecimal form [BEGIN,END) for VALUE by
> > + // round-tripping it through from_chars (if available).
> > + auto verify_via_from_chars = [] (char *begin, char *end, long double
> > value) {
> > +#if __cpp_lib_to_chars >= 201611L || _GLIBCXX_HAVE_USELOCALE
>
> This is currently going to fail, because we don't actually define
> __cpp_lib_to_chars yet (we should fix that!)
>
> Is checking the feature test macro here useful? We know that
> floating-point from_chars was committed before to_chars, so if this
> test is running, we should have from_chars (modulo uselocale being
> available, so that check is right). Is this to make the test usable
> for other C++ std::lib implementations?
This preprocessor check is copied from from_chars/{5,6}.cc, which I
figured should be appropriate to use here as well. I figured we'd
want to adjust each of these checks after we define __cpp_lib_to_chars
appropriately anyway (e.g. if __cpp_lib_to_chars is conditioned on
uselocale being available, then the three tests should be changed just
look at __cpp_lib_to_chars, IIUC).
>
> > + long double roundtrip;
> > + auto result = from_chars(begin, end, roundtrip, chars_format::hex);
> > + VERIFY( result.ec == errc{} );
> > + VERIFY( result.ptr == end );
> > + VERIFY( roundtrip == value );
> > +#endif
>
>
