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

[Marshall Clow via Phabricator via cfe-commits]

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 advantage - it doesn't depend on the parameters (template or runtime).




================
Comment at: src/support/itoa/itoa.cpp:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
lichray wrote:
> EricWF wrote:
> > This file should be renamed `charconv.cpp` and moved into the main source directory.
> We are going to have floating point cpp files so I don't think that one charconv.cpp is enough.
We can put both integral and floating point routines in the same source file ;-)

What Eric said - there should be just `charconv.cpp`, and no subdirectory.

Also, if this is not your work, then I need some notice (an email is fine) by the author saying that they're OK with contributing this under the LLVM license.


================
Comment at: src/support/itoa/itoa.cpp:35
+
+#define APPEND1(i)                              \
+    do                                          \
----------------
lichray wrote:
> EricWF wrote:
> > Any reason these can't be `static` functions? The compiler should optimize them away nicely.
> Although yes, but that's what the author provides.  It's an implementation file, so it doesn't matter I guess.
It *does* matter, since we'll have to maintain this.

It would also be nice if they had meaningful names.


Repository:
  rCXX libc++

https://reviews.llvm.org/D41458