[libc-commits] [PATCH] D125929: [libc] add printf base 10 integer conversion
Michael Jones via Phabricator via libc-commits
libc-commits at lists.llvm.org
Wed Jun 8 14:21:46 PDT 2022
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));
> 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
> 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.
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the libc-commits