From: Patrick Palka <ppalka@redhat.com>
To: Jonathan Wakely <jwakely@redhat.com>
Cc: Patrick Palka <ppalka@redhat.com>,
libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org
Subject: Re: [PATCH 1/2] libstdc++: Robustify long double std::to_chars testcase [PR98384]
Date: Wed, 24 Feb 2021 12:04:14 -0500 (EST) [thread overview]
Message-ID: <278c11aa-5d75-bdd2-bb9-4bd5d6c12f75@idea> (raw)
In-Reply-To: <20210224164404.GW3008@redhat.com>
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
>
>
next prev parent reply other threads:[~2021-02-24 17:04 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-22 21:55 Patrick Palka
2021-02-22 21:55 ` [PATCH 2/2] libstdc++: Fix endianness issue with IBM long double [PR98384] Patrick Palka
2021-02-22 22:22 ` Jonathan Wakely
2021-02-23 16:30 ` [PATCH 1/2] libstdc++: Robustify long double std::to_chars testcase [PR98384] Patrick Palka
2021-02-24 16:44 ` Jonathan Wakely
2021-02-24 17:04 ` Patrick Palka [this message]
2021-02-24 17:08 ` Jonathan Wakely
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=278c11aa-5d75-bdd2-bb9-4bd5d6c12f75@idea \
--to=ppalka@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jwakely@redhat.com \
--cc=libstdc++@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).