[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

This revision was automatically updated to reflect the committed changes. Closed by commit rL338479: [libc++][C++17] Elementary string conversions for integral types (authored by lichray, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

mclow.lists accepted this revision. mclow.lists added a comment. This revision is now accepted and ready to land. LGTM. Thanks! Repository: rCXX libc++ https://reviews.llvm.org/D41458 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray updated this revision to Diff 158404. lichray added a comment. Dropping a lambda Repository: rCXX libc++ https://reviews.llvm.org/D41458 Files: include/CMakeLists.txt include/charconv include/module.modulemap src/charconv.cpp test/libcxx/double_include.sh.cpp

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

Quuxplusone added a comment. @lichray: I'm interested in merging this patch into my libc++ fork, but the latest update seems to be missing a ton of files. For example `charconv_test_helpers.h` is included by one of the tests, but does not exist. Also there's a lot of boilerplate missing: you

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray marked an inline comment as done. lichray added inline comments. Comment at: include/charconv:158 + +#if !defined(_LIBCPP_COMPILER_MSVC) +static _LIBCPP_INLINE_VISIBILITY int __width(_Tp __v) mclow.lists wrote: > In general, we don't put

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

mclow.lists added a comment. Getting close here. I'll have a couple more comments later today, so don't post a new diff quite yet. Comment at: include/charconv:244 +static _LIBCPP_INLINE_VISIBILITY char const* +read(char const* __p, char const* __ep, type& __a, type&

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray added a comment. Ping. Any more comments? Repository: rCXX libc++ https://reviews.llvm.org/D41458 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray marked 2 inline comments as done. lichray added inline comments. Comment at: include/charconv:244 +static _LIBCPP_INLINE_VISIBILITY char const* +read(char const* __p, char const* __ep, type& __a, type& __b) +{ Quuxplusone wrote: > mclow.lists

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray updated this revision to Diff 155780. lichray added a comment. Uglify all the names Repository: rCXX libc++ https://reviews.llvm.org/D41458 Files: include/CMakeLists.txt include/charconv include/module.modulemap src/charconv.cpp test/libcxx/double_include.sh.cpp

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

Quuxplusone added inline comments. Comment at: include/charconv:244 +static _LIBCPP_INLINE_VISIBILITY char const* +read(char const* __p, char const* __ep, type& __a, type& __b) +{ mclow.lists wrote: > lichray wrote: > > mclow.lists wrote: > > > Same

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

mclow.lists added inline comments. Comment at: include/charconv:244 +static _LIBCPP_INLINE_VISIBILITY char const* +read(char const* __p, char const* __ep, type& __a, type& __b) +{ lichray wrote: > mclow.lists wrote: > > Same comment as above about

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

mclow.lists added inline comments. Comment at: include/charconv:372 + +auto __len = __last - __p; +if (__value != 0 || !__len) mclow.lists wrote: > Are you missing an edge case here? What happens if `__last == __first && > __value == 0`? > Nevermind.

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray added inline comments. Comment at: include/charconv:244 +static _LIBCPP_INLINE_VISIBILITY char const* +read(char const* __p, char const* __ep, type& __a, type& __b) +{ mclow.lists wrote: > Same comment as above about `read` and

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

mclow.lists added inline comments. Comment at: include/charconv:359 +auto __gen_digit = [](_Tp __c) { +return "0123456789abcdefghijklmnopqrstuvwxyz"[__c]; +}; Thinking some more - did this used to do more? Because I don't see why having a lambda

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

mclow.lists added inline comments. Comment at: include/charconv:244 +static _LIBCPP_INLINE_VISIBILITY char const* +read(char const* __p, char const* __ep, type& __a, type& __b) +{ Same comment as above about `read` and `inner_product` - they need to

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

mclow.lists added inline comments. Comment at: include/charconv:372 + +auto __len = __last - __p; +if (__value != 0 || !__len) Are you missing an edge case here? What happens if `__last == __first && __value == 0`? Comment at:

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray marked an inline comment as done. lichray added inline comments. Comment at: include/charconv:89 +_LIBCPP_BEGIN_NAMESPACE_STD + +enum class _LIBCPP_ENUM_VIS chars_format lichray wrote: > mclow.lists wrote: > > Quuxplusone wrote: > > > lichray wrote: > >

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray updated this revision to Diff 154842. lichray added a comment. Dropping C++11 support. Repository: rCXX libc++ https://reviews.llvm.org/D41458 Files: include/CMakeLists.txt include/charconv include/module.modulemap src/charconv.cpp test/libcxx/double_include.sh.cpp

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray marked an inline comment as done. lichray added inline comments. Comment at: include/charconv:234 +to_chars(char* __first, char* __last, _Tp __value, int __base) +-> to_chars_result +{ mclow.lists wrote: > lichray wrote: > > mclow.lists wrote: > > >

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray updated this revision to Diff 154816. lichray added a comment. Less trailing return types Repository: rCXX libc++ https://reviews.llvm.org/D41458 Files: include/CMakeLists.txt include/charconv include/module.modulemap src/charconv.cpp test/libcxx/double_include.sh.cpp

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray marked 3 inline comments as done. lichray added inline comments. Comment at: include/charconv:89 +_LIBCPP_BEGIN_NAMESPACE_STD + +enum class _LIBCPP_ENUM_VIS chars_format mclow.lists wrote: > Quuxplusone wrote: > > lichray wrote: > > > mclow.lists wrote:

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray updated this revision to Diff 154804. lichray marked an inline comment as done. lichray added a comment. Respond to the 2nd round review Repository: rCXX libc++ https://reviews.llvm.org/D41458 Files: include/CMakeLists.txt include/charconv include/module.modulemap

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

mclow.lists added inline comments. Comment at: .gitignore:7 *.so +*.core + I'm pretty sure this file doesn't belong in this diff. Comment at: include/charconv:89 +_LIBCPP_BEGIN_NAMESPACE_STD + +enum class _LIBCPP_ENUM_VIS chars_format

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray added inline comments. Comment at: test/support/charconv_test_helpers.h:24 + +template +constexpr auto mclow.lists wrote: > If this is supposed to be a C++17 or later header (and I'm pretty sure it > is), you should add a `static_assert(TEST_STD_VER >

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray updated this revision to Diff 153888. lichray added a comment. Install the header file Repository: rCXX libc++ https://reviews.llvm.org/D41458 Files: .gitignore include/CMakeLists.txt include/charconv include/module.modulemap src/charconv.cpp

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray marked an inline comment as done. lichray added inline comments. Comment at: src/charconv.cpp:34 + +#define APPEND1(i) \ +do \ lichray wrote: > mclow.lists wrote: > > I'd really

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray updated this revision to Diff 153868. lichray added a comment. Herald added a subscriber: ldionne. A macro-free implementation Repository: rCXX libc++ https://reviews.llvm.org/D41458 Files: .gitignore include/charconv include/module.modulemap src/charconv.cpp

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

Quuxplusone added inline comments. Comment at: include/charconv:89 +_LIBCPP_BEGIN_NAMESPACE_STD + +enum class _LIBCPP_ENUM_VIS chars_format lichray wrote: > mclow.lists wrote: > > lichray wrote: > > > EricWF wrote: > > > > We need to hide these names when

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray added inline comments. Comment at: include/charconv:89 +_LIBCPP_BEGIN_NAMESPACE_STD + +enum class _LIBCPP_ENUM_VIS chars_format mclow.lists wrote: > lichray wrote: > > EricWF wrote: > > > We need to hide these names when `_LIBCPP_STD_VER < 17`, since

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

mclow.lists added inline comments. Comment at: include/charconv:89 +_LIBCPP_BEGIN_NAMESPACE_STD + +enum class _LIBCPP_ENUM_VIS chars_format lichray wrote: > EricWF wrote: > > We need to hide these names when `_LIBCPP_STD_VER < 17`, since we're not > > allowed

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray updated this revision to Diff 141110. lichray added a comment. Update module map Repository: rCXX libc++ https://reviews.llvm.org/D41458 Files: .gitignore include/charconv include/module.modulemap src/charconv.cpp test/std/utilities/charconv/

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray updated this revision to Diff 139422. lichray added a comment. Reorganize files Repository: rCXX libc++ https://reviews.llvm.org/D41458 Files: .gitignore include/charconv src/charconv.cpp test/std/utilities/charconv/ test/std/utilities/charconv/charconv.from.chars/

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray marked 3 inline comments as done. lichray added inline comments. Comment at: include/charconv:234 +to_chars(char* __first, char* __last, _Tp __value, int __base) +-> to_chars_result +{ mclow.lists wrote: > Why use the trailing return type here? > I

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

mclow.lists added a comment. Sorry I've let this lie fallow for so long. Comment at: include/charconv:234 +to_chars(char* __first, char* __last, _Tp __value, int __base) +-> to_chars_result +{ Why use the trailing return type here? I don't see any

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray updated this revision to Diff 138842. lichray added a comment. Keep patches split Repository: rCXX libc++ https://reviews.llvm.org/D41458 Files: .gitignore include/charconv include/support/itoa/ include/support/itoa/itoa.h lib/CMakeLists.txt src/support/itoa/

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray updated this revision to Diff 138841. lichray marked an inline comment as not done. lichray added a comment. Addressing 'Done' comments Repository: rCXX libc++ https://reviews.llvm.org/D41458 Files: .gitignore include/__errc include/charconv include/support/itoa/

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray marked an inline comment as not done. lichray added inline comments. Comment at: include/support/itoa/itoa.h:29 +UINT64_C(1000), +UINT64_C(1), +UINT64_C(10), EricWF wrote: > The `UINT64_C` and `UINT32_C` macros are non-standard, so I

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray marked 7 inline comments as done. lichray added a comment. Herald added a subscriber: christof. Pending patch update due to poor network. Comment at: include/charconv:90 + +enum class _LIBCPP_ENUM_VIS chars_format +{ EricWF wrote: > enum types should

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray added a comment. I will pick up the changes later next week. Comment at: include/charconv:89 +_LIBCPP_BEGIN_NAMESPACE_STD + +enum class _LIBCPP_ENUM_VIS chars_format EricWF wrote: > We need to hide these names when `_LIBCPP_STD_VER < 17`, since we're

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

EricWF added a comment. @lichray I should have mentioned: Although this header is C++17 only, the bits compiled into the dylib need to compile as C++11 still. Repository: rCXX libc++ https://reviews.llvm.org/D41458 ___ cfe-commits mailing list

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

EricWF added a comment. Some initial thoughts. @mclow.lists Are you planning on moving forward with your implementation as well? Comment at: include/charconv:89 +_LIBCPP_BEGIN_NAMESPACE_STD + +enum class _LIBCPP_ENUM_VIS chars_format We need to hide these

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray updated this revision to Diff 129714. lichray added a comment. src/support/itoa/itoa.cpp in previous diffs were copyrighted by Tencent, now LLVM, contributed by the same author. Repository: rCXX libc++ https://reviews.llvm.org/D41458 Files: .gitignore include/charconv

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray updated this revision to Diff 128393. lichray added a comment. Just include math.h Repository: rCXX libc++ https://reviews.llvm.org/D41458 Files: .gitignore include/charconv include/support/itoa/ include/support/itoa/itoa.h lib/CMakeLists.txt src/support/itoa/

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

lichray updated this revision to Diff 128071. lichray added a comment. Use Marshall's method to generate digits in reversed order in generic to_chars. Repository: rCXX libc++ https://reviews.llvm.org/D41458 Files: .gitignore include/charconv include/support/itoa/