This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Fixes std::to_chars for bases != 10.
ClosedPublic

Authored by Mordante on Apr 18 2021, 5:30 AM.

Details

Summary

While working on D70631, Microsoft's unit tests discovered an issue.
Our std::to_chars implementation for bases != 10 uses the range
[first,last) as temporary buffer. This violates the contract for
to_chars:
[charconv.to.chars]/1 http://eel.is/c++draft/charconv#to.chars-1
to_chars_result to_chars(char* first, char* last, see below value, int base = 10);
"If the member ec of the return value is such that the value is equal to
the value of a value-initialized errc, the conversion was successful and
the member ptr is the one-past-the-end pointer of the characters
written."

Our implementation modifies the range [member ptr, last), which causes
Microsoft's test to fail. Their test verifies the buffer
[member ptr, last) is unchanged. (The test is only done when the
conversion is successful.)

While looking at the code I noticed the performance for bases != 10 also
is suboptimal. This is tracked in D97705.

This patch fixes the issue and adds a benchmark. This benchmark will be
used as baseline for D97705.

Diff Detail

Event Timeline

Mordante requested review of this revision.Apr 18 2021, 5:30 AM
Mordante created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2021, 5:30 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante updated this revision to Diff 338547.Apr 19 2021, 9:43 AM

Fixes signed vs unsigned compiler warnings.

Friendly ping.

Quuxplusone added a subscriber: Quuxplusone.

LGTM % nits. You probably want to ping Louis (or whoever) by name, though.

libcxx/include/charconv
405

Break line after int.
Also, it'd be nice to use the same 4-space indents in these two new functions as are used everywhere else in the file.

408–410

Nit: I'd have named these __base2, __base3, __base4 and expressed __base4 as __base2 * __base2.

439–444

IIUC, __diff is __cap and __width is __n? I think you should do the renaming.

libcxx/test/support/charconv_test_helpers.h
111

Might be even better to initialize with
for (int i=0; i < sizeof(buf); ++i) buf[i] = i+1;
and check with
for (int i=0; i < sizeof(buf); ++i) assert(buf[i] < r.ptr || buf[i] == i+1);
or something along those lines (I didn't think too hard about it), to verify that we're not even replacing part of the buffer with some shuffled other part of the buffer, or memsetting the buffer to buf[0], or anything dumb like that.
I can't think of any plausible mechanism that would cause such a bug, though, so maybe this doesn't matter.

Mordante marked 4 inline comments as done.Apr 27 2021, 10:37 AM

Thanks for the review!

libcxx/include/charconv
405

This is what clang-format makes of it. IMO if we don't like the style we should change our clang-format settings.

408–410

Renamed to __base_x.

439–444

Copy-paste programming ;-) Renamed them.

libcxx/test/support/charconv_test_helpers.h
111

I also can't think of a plausible reason. However the change is trivial so I've changed it.

Mordante updated this revision to Diff 340935.Apr 27 2021, 12:08 PM
Mordante marked 4 inline comments as done.

Addresses review comments.

zoecarver accepted this revision.Apr 29 2021, 10:31 AM
zoecarver added a subscriber: zoecarver.

Send it. Thanks!

This revision is now accepted and ready to land.Apr 29 2021, 10:31 AM
This revision was automatically updated to reflect the committed changes.