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

Siva Chandra via Phabricator via libc-commits libc-commits at lists.llvm.org
Tue Jun 7 11:58:20 PDT 2022

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
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?

  rG LLVM Github Monorepo



More information about the libc-commits mailing list