[libcxx-commits] [PATCH] D63047: [libc++] Fix leading zeros in std::to_chars

[Afanasyev Ivan via Phabricator via libcxx-commits]

ivafanas marked 12 inline comments as done.
ivafanas added inline comments.


================
Comment at: libcxx/src/charconv.cpp:67
+{
+    return v < 10 ? append1(buffer, v) : append2(buffer, v);
+}
----------------
lichray wrote:
> I suggest to use `if` here to align "append1" and "append2" closer to the condition
> ```
>     if (v < 10)
>         return append1(buffer, v);
>     else
>         return append2(buffer, v);
> ```
> 
> PS: I like the way how clang-format splits ternary operators,
> ```
>     return v < 10 ? append1(buffer, v)
>                   : append2(buffer, v);
> ```
> but here this line is too short to receive the above formatting.
Done


================
Comment at: libcxx/src/charconv.cpp:76
+    else
+        return v < 1000 ? append3(buffer, v) : append4(buffer, v);
+}
----------------
lichray wrote:
> Ditto, and else if should be enough (to align all calls to start from the same column).
Done


================
Comment at: libcxx/src/charconv.cpp:79
+
+inline _LIBCPP_INLINE_VISIBILITY char*
+append8_no_zeros(char* buffer, uint32_t v)
----------------
lichray wrote:
> I suggest to make these new routines templates as well, so even someone changes the logic in actually formatting code slightly "wrong," the code can still behave correctly.
Done


================
Comment at: libcxx/src/charconv.cpp:119
     {
-        uint32_t v = static_cast<uint32_t>(value);
-        if (v < 10000)
-        {
-            if (v < 100)
-            {
-                if (v < 10)
-                    buffer = append1(buffer, v);
-                else
-                    buffer = append2(buffer, v);
-            }
-            else
-            {
-                if (v < 1000)
-                    buffer = append3(buffer, v);
-                else
-                    buffer = append4(buffer, v);
-            }
-        }
-        else
-        {
-            // value = bbbbcccc
-            const uint32_t b = v / 10000;
-            const uint32_t c = v % 10000;
-
-            if (v < 1000000)
-            {
-                if (v < 100000)
-                    buffer = append1(buffer, b);
-                else
-                    buffer = append2(buffer, b);
-            }
-            else
-            {
-                if (v < 10000000)
-                    buffer = append3(buffer, b);
-                else
-                    buffer = append4(buffer, b);
-            }
-
-            buffer = append4(buffer, c);
-        }
+        buffer = append8_no_zeros(buffer, value);
     }
----------------
lichray wrote:
> Can you keep the static_cast?
Returned back


================
Comment at: libcxx/src/charconv.cpp:136
 
-        if (a < 100)
-        {
-            if (a < 10)
-                buffer = append1(buffer, a);
-            else
-                buffer = append2(buffer, a);
-        }
-        else
-        {
-            if (a < 1000)
-                buffer = append3(buffer, a);
-            else
-                buffer = append4(buffer, a);
-        }
+        buffer = append4_no_zeros(buffer, a);
 
----------------
lichray wrote:
> Can you keep the empty line after this line?
Do not understand, sorry.
There are already empty lines before and after `buffer = append4_no_zeros(buffer, a);`


================
Comment at: libcxx/test/std/utilities/charconv/charconv.to.chars/integral.pass.cpp:40
-        test(42, "42");
-        test(32768, "32768");
         test(0, "0", 10);
----------------
lichray wrote:
> Please keep these tests, because it's a sane test for ensuring omitting the "base" argument means base 10.
Done


================
Comment at: libcxx/test/std/utilities/charconv/charconv.to.chars/integral.pass.cpp:69
-        test(-1, "-1");
-        test(-12, "-12");
         test(-1, "-1", 10);
----------------
lichray wrote:
> Ditto.
Done


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63047/new/

https://reviews.llvm.org/D63047