From: Jonathan Wakely <jwakely@redhat.com>
To: Christophe Lyon <christophe.lyon@linaro.org>
Cc: libstdc++ <libstdc++@gcc.gnu.org>, gcc Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] Replace std::to_string for integers with optimized version
Date: Mon, 17 Jun 2019 13:21:00 -0000 [thread overview]
Message-ID: <20190617132113.GR643@redhat.com> (raw)
In-Reply-To: <CAKdteObYAcT1u7njC2x3uHCr16GbgkXhM-Gw5BEBvt7=oqRr4A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3836 bytes --]
On 13/06/19 22:41 +0200, Christophe Lyon wrote:
>Hi,
>
>
>On Wed, 12 Jun 2019 at 16:54, Jonathan Wakely <jwakely@redhat.com> wrote:
>>
>> The std::to_chars functions from C++17 can be used to implement
>> std::to_string with much better performance than calling snprintf. Only
>> the __detail::__to_chars_len and __detail::__to_chars_10 functions are
>> needed for to_string, because it always outputs base 10 representations.
>>
>> The return type of __detail::__to_chars_10 should not be declared before
>> C++17, so the function body is extracted into a new function that can be
>> reused by to_string and __detail::__to_chars_10.
>>
>> The existing tests for to_chars rely on to_string to check for correct
>> answers. Now that they use the same code that doesn't actually ensure
>> correctness, so add new tests for std::to_string that compare against
>> printf output.
>>
>> * include/Makefile.am: Add new <bits/charconv.h> header.
>> * include/Makefile.in: Regenerate.
>> * include/bits/basic_string.h (to_string(int), to_string(unsigned))
>> (to_string(long), to_string(unsigned long), to_string(long long))
>> (to_string(unsigned long long)): Rewrite to use __to_chars_10_impl.
>> * include/bits/charconv.h: New header.
>> (__detail::__to_chars_len): Move here from <charconv>.
>> (__detail::__to_chars_10_impl): New function extracted from
>> __detail::__to_chars_10.
>> * include/std/charconv (__cpp_lib_to_chars): Add, but comment out.
>> (__to_chars_unsigned_type): New class template that reuses
>> __make_unsigned_selector_base::__select to pick a type.
>> (__unsigned_least_t): Redefine as __to_chars_unsigned_type<T>::type.
>> (__detail::__to_chars_len): Move to new header.
>> (__detail::__to_chars_10): Add inline specifier. Move code doing the
>> output to __detail::__to_chars_10_impl and call that.
>> * include/std/version (__cpp_lib_to_chars): Add, but comment out.
>> * testsuite/21_strings/basic_string/numeric_conversions/char/
>> to_string.cc: Fix reference in comment. Remove unused variable.
>> * testsuite/21_strings/basic_string/numeric_conversions/char/
>> to_string_int.cc: New test.
>>
>> Tested x86_64-linux, committed to trunk.
>>
>
>The new test to_string_int.cc fails on arm-none-eabi:
>PASS: 21_strings/basic_string/numeric_conversions/char/to_string_int.cc
>(test for excess errors)
>spawn /aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/utils/bin/qemu-wrapper.sh
>./to_string_int.exe
>/libstdc++-v3/testsuite/21_strings/basic_string/numeric_conversions/char/to_string_int.cc:105:
>void check_value(T) [with T = long long int]: Assertion 's ==
>expected' failed.
>FAIL: 21_strings/basic_string/numeric_conversions/char/to_string_int.cc
>execution test
Does the target support "%lld" in printf?
Does the .sum file show UNSUPPORTED for the existing
21_strings/basic_string/numeric_conversions/char/to_string.cc test?
Does the attached patch make the test show what fails?
I suspect the problem is that the test relies on snprintf to check the
answers are correct, even though the actual library code doesn't need
snprintf any longer. Previously the std::to_string functions were all
guarded by _GLIBCXX_USE_C99_STDIO and so I'm guessing were not
supported for arm-none-eabi. Now the overloads for integral types
should work without any C library support, and so the new test is
enabled unconditionally. But the test itself still uses the C library.
Maybe I should have a simple test that doesn't use snprintf and just
checks some hardcoded values produce the expected results, and a more
involved test that compares the results to snprintf but uses
{ dg-require-string-conversions "" } like the existing test.
[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 994 bytes --]
diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/numeric_conversions/char/to_string_int.cc b/libstdc++-v3/testsuite/21_strings/basic_string/numeric_conversions/char/to_string_int.cc
index 8eb2a48bd30..911e6812b39 100644
--- a/libstdc++-v3/testsuite/21_strings/basic_string/numeric_conversions/char/to_string_int.cc
+++ b/libstdc++-v3/testsuite/21_strings/basic_string/numeric_conversions/char/to_string_int.cc
@@ -95,13 +95,15 @@ const std::uint_least32_t values[] = {
const std::size_t empty_string_capacity = std::string().capacity();
-#include <set>
+#include <iostream>
template<typename T>
void check_value(T val)
{
const std::string s = std::to_string(val);
const std::string expected = test::to_string(val);
+ if (s != expected)
+ std::cerr << "GOT: " << s << " BUT EXPECTED: " << expected << '\n';
VERIFY( s == expected );
VERIFY( s[s.size()] == '\0' ); // null-terminator not overwritten!
if (s.size() > empty_string_capacity)
next prev parent reply other threads:[~2019-06-17 13:21 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-12 14:54 Jonathan Wakely
2019-06-13 20:41 ` Christophe Lyon
2019-06-17 13:21 ` Jonathan Wakely [this message]
2019-06-18 11:58 ` Christophe Lyon
2019-06-18 12:41 ` 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=20190617132113.GR643@redhat.com \
--to=jwakely@redhat.com \
--cc=christophe.lyon@linaro.org \
--cc=gcc-patches@gcc.gnu.org \
--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).