Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions libc/src/stdio/printf_core/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ add_header_library(
parser.h
DEPENDS
.core_structs
libc.hdr.types.wint_t.h
libc.src.__support.arg_list
libc.src.__support.ctype_utils
libc.src.__support.str_to_integer
Expand Down Expand Up @@ -111,6 +112,9 @@ add_header_library(
.printf_config
.writer
libc.include.inttypes
libc.hdr.types.wchar_t
libc.hdr.types.wint_t
libc.hdr.wchar_macros
libc.src.__support.big_int
libc.src.__support.common
libc.src.__support.CPP.limits
Expand All @@ -123,6 +127,8 @@ add_header_library(
libc.src.__support.integer_to_string
libc.src.__support.libc_assert
libc.src.__support.uint128
libc.src.__support.wchar.mbstate
libc.src.__support.wchar.wcrtomb
libc.src.__support.StringUtil.error_to_string
libc.src.string.memory_utils.inline_memcpy
)
Expand Down
30 changes: 27 additions & 3 deletions libc/src/stdio/printf_core/char_converter.h
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//===-- String Converter for printf -----------------------------*- C++ -*-===//
//===-- Character Converter for printf --------------------------*- C++ -*-===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
Expand All @@ -9,7 +9,12 @@
#ifndef LLVM_LIBC_SRC_STDIO_PRINTF_CORE_CHAR_CONVERTER_H
#define LLVM_LIBC_SRC_STDIO_PRINTF_CORE_CHAR_CONVERTER_H

#include "hdr/types/wchar_t.h"
#include "hdr/types/wint_t.h"
#include "hdr/wchar_macros.h"
#include "src/__support/macros/config.h"
#include "src/__support/wchar/mbstate.h"
#include "src/__support/wchar/wcrtomb.h"
#include "src/stdio/printf_core/converter_utils.h"
#include "src/stdio/printf_core/core_structs.h"
#include "src/stdio/printf_core/writer.h"
Expand All @@ -21,7 +26,6 @@ template <WriteMode write_mode>
LIBC_INLINE int convert_char(Writer<write_mode> *writer,
const FormatSection &to_conv) {
char c = static_cast<char>(to_conv.conv_val_raw);

constexpr int STRING_LEN = 1;

size_t padding_spaces =
Expand All @@ -33,7 +37,27 @@ LIBC_INLINE int convert_char(Writer<write_mode> *writer,
RET_IF_RESULT_NEGATIVE(writer->write(' ', padding_spaces));
}

RET_IF_RESULT_NEGATIVE(writer->write(c));
if (to_conv.length_modifier == LengthModifier::l) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good overall, but we'll want to have a compile flag to disable wide characters to save code size. Could you add something similar to LIBC_COPT_PRINTF_DISABLE_STRERROR that just disables the wide character portion? You'll also need to guard the includes at the top of this file and the tests.

wint_t wi = static_cast<wint_t>(to_conv.conv_val_raw);

if (wi == WEOF) {
return -1;
}

char mb_str[MB_LEN_MAX];
internal::mbstate mbstate;
wchar_t wc = static_cast<wchar_t>(wi);

auto ret = internal::wcrtomb(mb_str, wc, &mbstate);
if (!ret.has_value()) {
return -1;
}

RET_IF_RESULT_NEGATIVE(writer->write({mb_str, ret.value()}));

} else {
RET_IF_RESULT_NEGATIVE(writer->write(c));
}

// If the padding is on the right side, write the spaces last.
if (padding_spaces > 0 &&
Expand Down
13 changes: 9 additions & 4 deletions libc/src/stdio/printf_core/parser.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#ifndef LLVM_LIBC_SRC_STDIO_PRINTF_CORE_PARSER_H
#define LLVM_LIBC_SRC_STDIO_PRINTF_CORE_PARSER_H

#include "hdr/types/wint_t.h"
#include "include/llvm-libc-macros/stdfix-macros.h"
#include "src/__support/CPP/algorithm.h" // max
#include "src/__support/CPP/limits.h"
Expand Down Expand Up @@ -73,9 +74,9 @@ template <typename ArgProvider> class Parser {
ArgProvider args_cur;

#ifndef LIBC_COPT_PRINTF_DISABLE_INDEX_MODE
// args_start stores the start of the va_args, which is allows getting the
// value of arguments that have already been passed. args_index is tracked so
// that we know which argument args_cur is on.
// args_start stores the start of the va_args, which helps in getting the
// number of arguments that have already been passed. args_index is tracked
// so that we know which argument args_cur is on.
ArgProvider args_start;
size_t args_index = 1;

Expand Down Expand Up @@ -173,7 +174,11 @@ template <typename ArgProvider> class Parser {
section.has_conv = true;
break;
case ('c'):
WRITE_ARG_VAL_SIMPLEST(section.conv_val_raw, int, conv_index);
if (section.length_modifier == LengthModifier::l) {
WRITE_ARG_VAL_SIMPLEST(section.conv_val_raw, wint_t, conv_index);
} else {
WRITE_ARG_VAL_SIMPLEST(section.conv_val_raw, int, conv_index);
}
break;
case ('d'):
case ('i'):
Expand Down
3 changes: 3 additions & 0 deletions libc/test/src/stdio/printf_core/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ add_libc_unittest(
LINK_LIBRARIES
LibcPrintfHelpers
DEPENDS
libc.hdr.types.wchar_t
libc.src.stdio.printf_core.parser
libc.src.stdio.printf_core.core_structs
libc.src.__support.CPP.string_view
Expand All @@ -32,6 +33,8 @@ add_libc_unittest(
SRCS
converter_test.cpp
DEPENDS
libc.hdr.types.wchar_t
libc.hdr.wchar_macros
libc.src.stdio.printf_core.converter
libc.src.stdio.printf_core.writer
libc.src.stdio.printf_core.core_structs
Expand Down
83 changes: 82 additions & 1 deletion libc/test/src/stdio/printf_core/converter_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@
//
//===----------------------------------------------------------------------===//

#include "hdr/types/wchar_t.h"
#include "hdr/wchar_macros.h"
#include "src/stdio/printf_core/converter.h"
#include "src/stdio/printf_core/core_structs.h"
#include "src/stdio/printf_core/writer.h"

#include "test/UnitTest/Test.h"

class LlvmLibcPrintfConverterTest : public LIBC_NAMESPACE::testing::Test {
Expand Down Expand Up @@ -255,3 +256,83 @@ TEST_F(LlvmLibcPrintfConverterTest, OctConversion) {
ASSERT_STREQ(str, "1234");
ASSERT_EQ(writer.get_chars_written(), size_t{4});
}

TEST_F(LlvmLibcPrintfConverterTest, WideCharConversion) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For simplicity most of the functionality testing of the printf conversions is actually done in sprintf_test.cpp. Doing it that way makes the tests less verbose and more accurate to their actual use. I would recommend removing the tests from converter_test.cpp and only having the wide character tests in sprintf_test.cpp.

LIBC_NAMESPACE::printf_core::FormatSection section;
section.has_conv = true;
section.raw_string = "%lc";
section.conv_name = 'c';
section.length_modifier = LIBC_NAMESPACE::printf_core::LengthModifier::l;
section.conv_val_raw = static_cast<wchar_t>(L'€');

LIBC_NAMESPACE::printf_core::convert(&writer, section);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should also consider adding end-to-end test of this functionality - e.g. a test of snprintf invocation which prints converted wide characters to a multi-byte sting, and a test case in libc/test/src/stdio/printf_core/parser_test.cpp to ensure that %lc is parsed correctly.

For example, the Parser should accept the arguments of type wint_t in this case, according to specification - so probably you'd need to update the parsing code.


wb.buff[wb.buff_cur] = '\0';

ASSERT_STREQ(str, "€");
ASSERT_EQ(writer.get_chars_written(), size_t{3});
}

TEST_F(LlvmLibcPrintfConverterTest, WideCharConversionLeftJustified) {
LIBC_NAMESPACE::printf_core::FormatSection left_justified_conv;
left_justified_conv.has_conv = true;
left_justified_conv.raw_string = "%-4lc";
left_justified_conv.conv_name = 'c';
left_justified_conv.length_modifier =
LIBC_NAMESPACE::printf_core::LengthModifier::l;
left_justified_conv.flags =
LIBC_NAMESPACE::printf_core::FormatFlags::LEFT_JUSTIFIED;
left_justified_conv.min_width = 4;
left_justified_conv.conv_val_raw = static_cast<wchar_t>(L'€');

LIBC_NAMESPACE::printf_core::convert(&writer, left_justified_conv);
wb.buff[wb.buff_cur] = '\0';

ASSERT_STREQ(str, "€ ");
ASSERT_EQ(writer.get_chars_written(), size_t{6});
}

TEST_F(LlvmLibcPrintfConverterTest, WideCharConversionRightJustified) {
LIBC_NAMESPACE::printf_core::FormatSection right_justified_conv;
right_justified_conv.has_conv = true;
right_justified_conv.raw_string = "%4lc";
right_justified_conv.conv_name = 'c';
right_justified_conv.length_modifier =
LIBC_NAMESPACE::printf_core::LengthModifier::l;
right_justified_conv.min_width = 4;
right_justified_conv.conv_val_raw = static_cast<wchar_t>(L'€');

LIBC_NAMESPACE::printf_core::convert(&writer, right_justified_conv);
wb.buff[wb.buff_cur] = '\0';

ASSERT_STREQ(str, " €");
ASSERT_EQ(writer.get_chars_written(), size_t{6});
}

TEST_F(LlvmLibcPrintfConverterTest, WideCharConversionInvalid) {
LIBC_NAMESPACE::printf_core::FormatSection section;
section.has_conv = true;
section.raw_string = "%lc";
section.conv_name = 'c';
section.length_modifier = LIBC_NAMESPACE::printf_core::LengthModifier::l;
// An invalid wide character.
section.conv_val_raw = static_cast<wchar_t>(0xFFFFFFFF);

int ret = LIBC_NAMESPACE::printf_core::convert(&writer, section);

ASSERT_EQ(ret, -1);
}

TEST_F(LlvmLibcPrintfConverterTest, WideCharWEOFConversion) {
LIBC_NAMESPACE::printf_core::FormatSection section;
section.has_conv = true;
section.raw_string = "%lc";
section.conv_name = 'c';
section.length_modifier = LIBC_NAMESPACE::printf_core::LengthModifier::l;
// WEOF value.
section.conv_val_raw = static_cast<wchar_t>(WEOF);

int ret = LIBC_NAMESPACE::printf_core::convert(&writer, section);

ASSERT_EQ(ret, -1);
}
19 changes: 19 additions & 0 deletions libc/test/src/stdio/printf_core/parser_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
//
//===----------------------------------------------------------------------===//

#include "hdr/types/wchar_t.h"
#include "src/__support/CPP/bit.h"
#include "src/__support/CPP/string_view.h"
#include "src/__support/arg_list.h"
Expand Down Expand Up @@ -370,6 +371,24 @@ TEST(LlvmLibcPrintfParserTest,
ASSERT_PFORMAT_EQ(expected, format_arr[0]);
}

TEST(LlvmLibcPrintfParserTest, EvalOneArgWithWideCharacter) {
LIBC_NAMESPACE::printf_core::FormatSection format_arr[2];
const char *str = "%lc";
wchar_t arg1 = L'€';
evaluate(format_arr, str, arg1);

LIBC_NAMESPACE::printf_core::FormatSection expected;
expected.has_conv = true;

expected.raw_string = {str, 3};
expected.length_modifier = LIBC_NAMESPACE::printf_core::LengthModifier::l;
expected.conv_val_raw =
static_cast<LIBC_NAMESPACE::fputil::FPBits<double>::StorageType>(arg1);
expected.conv_name = 'c';

ASSERT_PFORMAT_EQ(expected, format_arr[0]);
}

#ifndef LIBC_COPT_PRINTF_DISABLE_INDEX_MODE

TEST(LlvmLibcPrintfParserTest, IndexModeOneArg) {
Expand Down
Loading