On 24/02/21 12:04 -0500, Patrick Palka via Libstdc++ wrote:
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
I was going to say "which idiot wrote that then?" and then I realised that the check is fine and I just misread the || as &&. Doh.
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).
Agreed. The patch is fine for trunk, sorry for the noise. Thanks.
