[libc-commits] [PATCH] D125929: [libc] add printf base 10 integer conversion

[Siva Chandra via Phabricator via libc-commits]

sivachandra accepted this revision.
sivachandra added a comment.
This revision is now accepted and ready to land.

LGTM - But, if some of the changes to the tests are unrelated, then do it separately.



================
Comment at: libc/src/stdio/printf_core/int_converter.h:19
+
+void convert_int(Writer *writer, const FormatSection &to_conv) {
+  static constexpr size_t BITS_IN_BYTE = 8;
----------------
Does this have to be in a header file? If yes, then we should mark it inline and also add header guards. You should probably do that with other `<converter>.h` files?


================
Comment at: libc/src/stdio/printf_core/int_converter.h:41
+    // conversion is unsigned.
+    flags = static_cast<FormatFlags>(
+        flags & ~(FormatFlags::FORCE_SIGN | FormatFlags::SPACE_PREFIX));
----------------
Nit: Do you need the static cast?


================
Comment at: libc/src/stdio/printf_core/int_converter.h:127
+    } else {
+      // If there are enough digits to pass over the precision, just write the
+      // number, padded by spaces.
----------------
Nit: The first word starts with a captilized letter. Can we follow the same convention with all comments please?


================
Comment at: libc/test/src/stdio/printf_core/converter_test.cpp:27
       __llvm_libc::printf_core::write_to_string);
+};
 
----------------
I am probably missing something - how are these changes to string conversion tests related to integer conversion? Also, do we really want a separate string converter and writer for every test? If not, they can be static globals.

Another point, which can be addressed separately - we should probably not be testing against internal API for a converter, writer etc. You can have a test for verifying that the plumbing works, but everything else should probably be tested using `sprintf`.


================
Comment at: libc/test/src/stdio/printf_core/converter_test.cpp:185
+
+TEST_F(LlvmLibcPrintfConverterTest, IntConversionSignedPositive) {
+
----------------
Should we put these integer conversion tests in a separate file?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125929