Proposal: Allow String-Number Conversion Functions

[Giovanni Ortuño]

e.g. std::to_string().

Currently under "To be Discussed" and it's already used in quite a few places. Is there any reason why we shouldn't allow them?

Gio

[Scott Graham]

I found I couldn't replace dmg_fp with strtod() https://bugs.chromium.org/p/chromium/issues/detail?id=593512#c5 and ended up just adding some test cases.

So we should be cautious in the str->num direction, at least. (That's not to say that the dmg_fp code is great either, so if we test and discover we can remove it, that'd be great.)

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAMDKGBXM6meaZzGT34q6nnRMBmPxw%3D3muxY1uhzmh8vxc60JmQ%40mail.gmail.com.

[Peter Kasting]

On Thu, Jun 15, 2017 at 3:14 PM, Giovanni Ortuño <ort...@chromium.org> wrote:

e.g. std::to_string().

Currently under "To be Discussed" and it's already used in quite a few places. Is there any reason why we shouldn't allow them?

The row in the table links to https://bugs.chromium.org/p/chromium/issues/detail?id=593512 .  Did you read it?

In short, I'm in favor of using the standard library functions, but there are a lot of subtle gotchas here, and converting involves checking edge case behavior closely and making sure we have good tests for this.  Until then, people should not be using things that have not been OKed.

It's possible that we could choose to allow to_string() and not the others as a stepping stone, since I imagine that's much less fraught with peril.

PK

[Giovanni Ortuño]

I didn't read the linked issue close enough. Sorry about that.

Are there any objections to allowing to_string() as a stepping stone? It is used all over the place already.

Gio

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFAUyj_5Xd9UjfQecC4ii%2ByCP8Y8Oi0dXn5ZCSVveoDV8Q%40mail.gmail.com.

[Peter Kasting]

On Thu, Jun 15, 2017 at 3:44 PM, 'Giovanni Ortuño' via cxx <c...@chromium.org> wrote:

I didn't read the linked issue close enough. Sorry about that.

Are there any objections to allowing to_string() as a stepping stone? It is used all over the place already.

What are the existing mechanisms we use besides this?  We should make sure there aren't going to be gotchas if people convert one to another, e.g. locale formatting differences or something.

PK 

[Giovanni Ortuño]

We have a set of helper methods in base/strings/string_number_conversions.h to convert from different types of integers to string. Replacing their implementation with std::to_string didn't cause any tests to fail. Note that this only applies to integers, replacing the implementation of base::DoubleToString() does break tests.

But I found two reasons as to why we might want to disallow std::to_string():

1. Depends on std::locale for formatting. This might only apply to non-integers though.

2. It's quite slow. I did some rudimentary perf tests (spreadsheet, CL):

base::IntToString vs. std::to_string (100 times):

  - 120.41% increase for numbers between 0 and 1,000

  - 191.36% increase for numbers between -1,000 and 0

  - 151.53% increase for numbers between 100,000 and 1,000,000

  - 141.75% increase for numbers between -1,000,000 and -100,000

  

base::SizeTToString vs. std::to_string (100 times):

  - 176.00% increase for numbers between 0 and 1,000

  - 157.08% increase for numbers between 100,000 and 1,000,000

If we end up disallowing it, we should look into adding a presubmit check for it. IMO it is too easy to introduce uses of std::to_string as it's the first result when searching "int to string c++".

jyasskin, who also helped me think through this, mentioned that we should have a more convenient to-string function that's just no std::to_string(). Maybe we should just overload IntToString() to make it easier to use.

Gio

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFD6WdMCh5rkgcmorDxg%2B51G9By9UB89%2B5UsfG1pRMxhYQ%40mail.gmail.com.

[Yuta Kitamura]

On Fri, Jun 16, 2017 at 12:46 PM, Giovanni Ortuño <ort...@chromium.org> wrote:

We have a set of helper methods in base/strings/string_number_conversions.h to convert from different types of integers to string. Replacing their implementation with std::to_string didn't cause any tests to fail. Note that this only applies to integers, replacing the implementation of base::DoubleToString() does break tests.

But I found two reasons as to why we might want to disallow std::to_string():

1. Depends on std::locale for formatting. This might only apply to non-integers though.

2. It's quite slow. I did some rudimentary perf tests (spreadsheet, CL):

Agreed. Locale-dependent functions tend to be slow (like isupper() etc.), especially on non-ASCII locales. If you set LANG=C, the performance could be competitive (I guess), but that's not an option for us.