Page MenuHomePhabricator

[libc++] Fix leading zeros in std::to_chars
ClosedPublic

Authored by ivafanas on Jun 8 2019, 7:19 AM.

Details

Summary

It is a bugfix proposal for https://bugs.llvm.org/show_bug.cgi?id=42166.

std::to_chars appends leading zeros if input 64-bit value has 9, 10 or 11 digits.
According to documentation std::to_chars must not append leading zeros:
https://en.cppreference.com/w/cpp/utility/to_chars

Changeset should not affect std::to_chars performance:
http://quick-bench.com/CEpRs14xxA9WLvkXFtaJ3TWOVAg

Unit test that std::from_chars supports compatibility for both std::to_chars outputs (previous and fixed one) already exists:
https://github.com/llvm-mirror/libcxx/blob/1f60111b597e5cb80a4513ec86f79b7e137f7793/test/std/utilities/charconv/charconv.from.chars/integral.pass.cpp#L63

Diff Detail

Repository
rL LLVM

Event Timeline

ivafanas created this revision.Jun 8 2019, 7:19 AM
zoecarver added a subscriber: zoecarver.EditedJun 8 2019, 10:54 AM

The example in the bug doesn't work: std::to_chars(buf, buf + 100, (unsigned long)0xffffffff).

Edit: it outputs 0004294967295.

The example in the bug doesn't work: std::to_chars(buf, buf + 100, (unsigned long)0xffffffff).

Edit: it outputs 0004294967295.

Hi,

Hmm, are you sure?
I've just tested fixed version of std::to_chars and std::to_chars(buf, buf + 100, (unsigned long)0xffffffff) produces 4294967295 as expected.

May be you didn't recompile implementation before run lit tests?

I like the refactoring. I suggest not to drop existing (possibly redundant) tests.

libcxx/src/charconv.cpp
67 ↗(On Diff #203688)

I suggest to use if here to align "append1" and "append2" closer to the condition

if (v < 10)
    return append1(buffer, v);
else
    return append2(buffer, v);

PS: I like the way how clang-format splits ternary operators,

return v < 10 ? append1(buffer, v)
              : append2(buffer, v);

but here this line is too short to receive the above formatting.

76 ↗(On Diff #203688)

Ditto, and else if should be enough (to align all calls to start from the same column).

79 ↗(On Diff #203688)

I suggest to make these new routines templates as well, so even someone changes the logic in actually formatting code slightly "wrong," the code can still behave correctly.

119 ↗(On Diff #203688)

Can you keep the static_cast?

136 ↗(On Diff #203688)

Can you keep the empty line after this line?

libcxx/test/std/utilities/charconv/charconv.to.chars/integral.pass.cpp
40 ↗(On Diff #203688)

Please keep these tests, because it's a sane test for ensuring omitting the "base" argument means base 10.

69 ↗(On Diff #203688)

Ditto.

The example in the bug doesn't work: std::to_chars(buf, buf + 100, (unsigned long)0xffffffff).

Edit: it outputs 0004294967295.

Hi,

Hmm, are you sure?
I've just tested fixed version of std::to_chars and std::to_chars(buf, buf + 100, (unsigned long)0xffffffff) produces 4294967295 as expected.

May be you didn't recompile implementation before run lit tests?

Yep, you're right. I applied the patch but forgot to recompile. Sorry for the false alarm. This patch looks good to me :)

ivafanas updated this revision to Diff 203724.Jun 9 2019, 12:15 AM

Code review issues are adressed

ivafanas marked 12 inline comments as done.Jun 9 2019, 12:22 AM
ivafanas added inline comments.
libcxx/src/charconv.cpp
67 ↗(On Diff #203688)

Done

76 ↗(On Diff #203688)

Done

79 ↗(On Diff #203688)

Done

119 ↗(On Diff #203688)

Returned back

136 ↗(On Diff #203688)

Do not understand, sorry.
There are already empty lines before and after buffer = append4_no_zeros(buffer, a);

libcxx/test/std/utilities/charconv/charconv.to.chars/integral.pass.cpp
40 ↗(On Diff #203688)

Done

69 ↗(On Diff #203688)

Done

lichray accepted this revision.Jun 9 2019, 12:49 AM
lichray added inline comments.
libcxx/src/charconv.cpp
136 ↗(On Diff #203688)

This revision is good, don't worry.

This revision is now accepted and ready to land.Jun 9 2019, 12:49 AM

Ping @mclow.lists
Do you plan to do a final scan before I merging this?

Ping @mclow.lists
Do you plan to do a final scan before I merging this?

Yes. This morning.

mclow.lists accepted this revision.Jun 10 2019, 8:35 AM

LGTM, thanks. I have more tests that I'll be adding in the next week or so, but no reason to hold this up for them.

lichray updated this revision to Diff 203856.Jun 10 2019, 10:06 AM

Regenerate the patch from libcxx directory

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2019, 10:09 AM

@ivafanas I forgot to update the description to include a "thank you" in the commit message; sorry about that (