This patch adds support for d, i, and u conversions in printf, as well
as comprehensive unit tests.
Details
- Reviewers
sivachandra lntue - Commits
- rG1be3669dda4d: [libc] add printf base 10 integer conversion
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libc/src/stdio/printf_core/int_converter.h | ||
---|---|---|
25 | Seems like this one line function is used only once? | |
39 | BUFF_LEN | |
40 | Can this assignment ever lead to a truncation? May no because this is a convert_int which does not deal with integers wider than 64-bit? | |
60 | Why are these masking operations required? Also, for max values, we have: https://github.com/llvm/llvm-project/blob/main/libc/src/__support/CPP/Limits.h | |
91 | for (; num > 0; --buff_cur, num /= 10) buffer[buff_curr] = (num % 10) + '0'; |
libc/src/stdio/printf_core/int_converter.h | ||
---|---|---|
40 | It can't, because uintmax_t is at least as large as the largest valid integer type for printf. The size of conv_val_raw may be larger than uintmax_t to fit long double values, but the value passed by the user can't be larger than uintmax_t. | |
60 | This is to handle the fact that the length modifiers allow the user to specify the size of the integer to be converted. The type in num is always uintmax_t since that can fit any valid type, but the user can specify that they want it to be treated as a short, in which case we have to limit the value of num to what can fit in a short. The way this is done here is by creating a mask of just the bits in the specified type. |
libc/src/stdio/printf_core/int_converter.h | ||
---|---|---|
60 | Thanks for explaining. You can either use LLVM libc's own NumericLimits or directly use macros from the freestanding limits.h header. NumericLimits is probably more appropriate as you can get max values of size_t and ptrdiff_t is a straightforward way. |
switch to using limits.h for type size masks, additionally adjust the buffer size to be more exact.
LGTM - But, if some of the changes to the tests are unrelated, then do it separately.
libc/src/stdio/printf_core/int_converter.h | ||
---|---|---|
20 | 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? | |
42 | Nit: Do you need the static cast? | |
128 | Nit: The first word starts with a captilized letter. Can we follow the same convention with all comments please? | |
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. | |
185 | Should we put these integer conversion tests in a separate file? |
libc/test/src/stdio/printf_core/converter_test.cpp | ||
---|---|---|
27 | I forgot that StringWriter is stateful - ignore that part of the comment about needing a new string writer for every test. |
move tests into sprintf_tests and split converter_test changes into a separate patch
libc/src/stdio/printf_core/int_converter.h | ||
---|---|---|
42 | yes. The output of the arithmetic and is an int which doesn't have a default conversion to FormatFlags | |
libc/test/src/stdio/printf_core/converter_test.cpp | ||
27 | 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. |
address comments and add simple converter test
libc/src/stdio/printf_core/string_converter.h | ||
---|---|---|
8–11 | yes, I meant to add those and forgot. |
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?