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

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
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));
----------------
sivachandra wrote:
> Nit: Do you need the static cast?
yes. The output of the arithmetic and is an `int` which doesn't have a default conversion to `FormatFlags`


================
Comment at: libc/test/src/stdio/printf_core/converter_test.cpp:27
       __llvm_libc::printf_core::write_to_string);
+};
 
----------------
sivachandra wrote:
> sivachandra wrote:
> > 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`.
> I forgot that `StringWriter` is stateful - ignore that part of the comment about needing a new string writer for every test.
I think that having some tests specific to the writer, parser, and converter is helpful for determining which piece has the bug, although we can move primary functionality testing to sprintf for simplicity.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125929