This is an archive of the discontinued LLVM Phabricator instance.

[wip][libc] Overload integer_to_string methods for fixed size buffers
Needs ReviewPublic

Authored by JonChesterfield on Jul 20 2023, 5:08 PM.

Details

Summary

Existing integer_to_string methods return an optional<view>.
In the common case for a perfectly sized buffer, we can return a view.

WIP because I haven't built it for non-amdgpu yet and the wider than T
test is appending a spurious zero at present so some test cases fail.

The wider-than-T hex case has a comment for an invariant, planning to
make that a compile time fault as part of this.

Posting in WIP state as a sanity check that exact overloads are acceptable
The char (&buffer) [N] is for fixed size buffer of unknown contents, it
might be possible to detect a constexpr size string view. Ideas welcome.

Diff Detail

Event Timeline

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 20 2023, 5:08 PM
JonChesterfield requested review of this revision.Jul 20 2023, 5:08 PM
libc/src/__support/integer_to_string.h
174

this change ^ really should be a no-op, but seems to be changing the result on amdgpu. That might be a compiler bug.

libc/test/UnitTest/LibcTest.cpp
53

this ^ looks like the same bug from D155899, but might actually be fine. Placing __attribute__((warn_unused_result)) on the conversions shows a couple of places. It's possible the wider-than-8 hex case is intended to return values via the passed buffer, not the returned string view. I need to take a closer look.

gchatelet added inline comments.Jul 25 2023, 8:13 AM
libc/src/__support/libc_assert.h
35

[nit] cpp::string_view let's be explicit

libc/test/UnitTest/LibcTest.cpp
52

Not directly related to this patch but I think we should offer prefixing as part of the API. This would simplify client code.

libc/test/src/__support/integer_to_string_test.cpp
22

The required size changes if we pass in an array or a span, this is confusing.