This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Make to_chars base 10 header only.
ClosedPublic

Authored by Mordante on May 16 2022, 10:18 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rGa15ae4139cea: [libc++] Make to_chars base 10 header only.
Summary

The functions to_chars and from_chars should offer 128-bit support. This
is the first step to implement 128-bit version of to_chars. Before
implementing 128-bit support the current code will be polished.

This moves the code from the dylib to the header in prepartion of

P2291 "Add Constexpr Modifiers to Functions to_chars and from_chars for
Integral Types in <charconv> Header"

Note some more cleanups will be done in follow-up commits

  • Remove the _LIBCPP_AVAILABILITY_TO_CHARS from to_chars. With all code in the header the availablilty macro is no longer needed. This requires enabling the unit tests for additional platforms.
  • The code in the dylib can switch to using the header implementation. This allows removing the code duplicated in the header and the dylib.

Diff Detail

Event Timeline

Mordante created this revision.May 16 2022, 10:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2022, 10:18 AM
Herald added a subscriber: mgorny. · View Herald Transcript
Mordante requested review of this revision.May 16 2022, 10:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2022, 10:18 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik added inline comments.
libcxx/include/charconv
108–111

Since we have to keep the functions in the dylib forever anyways, why not use them?

Mordante added inline comments.May 16 2022, 11:11 AM
libcxx/include/charconv
108–111

Since I want to remove the code from the dylib I thought it was easier to use a new name instead of using the gnu_inline attribute.

* The code in the dylib can switch to using the header implementation. This allows removing the code duplicated in the header and the dylib.

We can even removed the code in the dylib for ABI v2.

Mordante updated this revision to Diff 429805.May 16 2022, 11:58 AM

Fixes CI errors.

Mordante updated this revision to Diff 429818.May 16 2022, 12:49 PM

Keep exporting the ABI functions.

ldionne accepted this revision.May 31 2022, 9:21 AM
ldionne added a subscriber: ldionne.

LGTM!

libcxx/include/__charconv/to_chars_base_10.h
36

We could be more ADL-safe here and elsewhere in this file. I guess it's mostly a matter of style and consistency because the types we use these functions with are not going to do ADL hijacking.

libcxx/include/charconv
160

Pre-existing issue but we might as well fix it -- this should be ADL safe. Same elsewhere.

This revision is now accepted and ready to land.May 31 2022, 9:21 AM
ldionne added inline comments.May 31 2022, 9:32 AM
libcxx/include/__charconv/tables.h
25

What we want is essentially inline constexpr char __digits_base_10[200], but we can't have that because we need to support C++11. IIRC, the canonical way to do this pre-C++17 was:

template <class = void>
struct __digits_base_10 {
  static constexpr char __value[200];
};

template <class _Tp>
char __digits_base_10<_Tp>::__value[200] = {
  // ...
};

Then, you access it as __digits_base_10<>::__value. It's not as pretty, but it gets you the linkonce_odr semantics that you are after even before C++17.

Note from live review: We may want to apply the same kind of transformation to the other places where we have this pattern in <charconv>. Using static constexpr might lead to code bloat.

37

Nit: it's a bit weird that this clang-format comment is not aligned with the previous one.

Mordante updated this revision to Diff 433431.Jun 1 2022, 9:34 AM
Mordante marked 4 inline comments as done.

Rebased and addresses review comments.

Mordante added inline comments.Jun 1 2022, 9:35 AM
libcxx/include/__charconv/tables.h
25

I did this place only for now. I'll have a look at the other places in a separated commit.

37

Agreed.

This revision was automatically updated to reflect the committed changes.