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

[Zhihao Yuan via Phabricator via cfe-commits]

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 `inner_product` - they need to be "ugly names"
Unlike `traits` which is a template parameter name in the standard, `read` and `inner_product` are function names in the standard, which means the users cannot make a macro for them (and there is no guarantee about what name you make **not** get by including certain headers), so we don't need to use ugly names here, am I right?


================
Comment at: include/charconv:358
+
+    auto __gen_digit = [](_Tp __c) {
+        return "0123456789abcdefghijklmnopqrstuvwxyz"[__c];
----------------
mclow.lists wrote:
> I just want you to reassure me here - this lambda gets inlined, right?
Yes -- I read my code in assembly.


================
Comment at: include/charconv:359
+    auto __gen_digit = [](_Tp __c) {
+        return "0123456789abcdefghijklmnopqrstuvwxyz"[__c];
+    };
----------------
mclow.lists wrote:
> Thinking some more - did this used to do more? Because I don't see why having a lambda here is a benefit, as compared to:
> 
>     const char *__digits = "0123456789abcdefghijklmnopqrstuvwxyz";
> 
> and
>     *--p = digits[__c];
> 
I use a lambda here because it may do more in the future.  If someone wants to let it support non-ASCII platforms, then they only need to make a patch against this lambda rather than changing the sites of uses.  After all, there is nothing wrong to abstract out anything into a function, I think...


Repository:
  rCXX libc++

https://reviews.llvm.org/D41458