Page MenuHomePhabricator

[libc++] Refactors charconv data tables.
Needs RevisionPublic

Authored by Mordante on Aug 14 2022, 4:08 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Summary

Currently the data tables can't be used in a constexpr function. This is
required to implement P2291R3 "Add Constexpr Modifiers to Functions
to_chars and from_chars for Integral Types in <charconv> Header"

Since inline constexpr variables are supported since C++17 use this them
starting from C++17. Before use the old tables and a reference helper
variable.

Diff Detail

Event Timeline

Mordante created this revision.Aug 14 2022, 4:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2022, 4:08 AM
Mordante requested review of this revision.Aug 14 2022, 4:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2022, 4:08 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante updated this revision to Diff 452516.Aug 14 2022, 5:54 AM

Use macros to fix the CI failures.

Mordante updated this revision to Diff 456132.Aug 27 2022, 9:52 AM

Rebased to test CI.

Mordante updated this revision to Diff 456688.Aug 30 2022, 8:27 AM

Attempts to fix the CI.

Mordante added inline comments.Aug 30 2022, 11:26 AM
libcxx/include/__charconv/to_chars_base_10.h
39

However this runs into https://github.com/llvm/llvm-project/issues/52954
I have added a comment in the source, which I will update on the next revision or when landing.

The same for similar occurrences in this file.

Mordante marked an inline comment as not done.Aug 30 2022, 11:29 AM
ldionne requested changes to this revision.Wed, Aug 31, 9:48 AM
ldionne added a subscriber: ldionne.

I am extremely tempted to simply drop support for to_chars and from_chars before C++17 so that none of this is necessary anymore. Would you be willing to create a patch for that? I could run it through some tests to gauge the impact and we can see whether that's viable for LLVM 16.

libcxx/include/__charconv/tables.h
45
This revision now requires changes to proceed.Wed, Aug 31, 9:48 AM

I am extremely tempted to simply drop support for to_chars and from_chars before C++17 so that none of this is necessary anymore. Would you be willing to create a patch for that? I could run it through some tests to gauge the impact and we can see whether that's viable for LLVM 16.

I've created D133216 to test this.