Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc] Support C23 'b' (binary) modifier in printf #80851

Merged
merged 9 commits into from
Feb 7, 2024

Conversation

agentcooper
Copy link
Contributor

@llvmbot llvmbot added the libc label Feb 6, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 6, 2024

@llvm/pr-subscribers-libc

Author: Artem Tyurin (agentcooper)

Changes

Reference: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2612.pdf.

Fixes #80727.


Full diff: https://github.com/llvm/llvm-project/pull/80851.diff

4 Files Affected:

  • (modified) libc/src/stdio/printf_core/converter.cpp (+2)
  • (modified) libc/src/stdio/printf_core/int_converter.h (+5-1)
  • (modified) libc/src/stdio/printf_core/parser.h (+4)
  • (modified) libc/test/src/stdio/printf_core/converter_test.cpp (+14)
diff --git a/libc/src/stdio/printf_core/converter.cpp b/libc/src/stdio/printf_core/converter.cpp
index 74a36cbf7432fb..52412aef3c5c15 100644
--- a/libc/src/stdio/printf_core/converter.cpp
+++ b/libc/src/stdio/printf_core/converter.cpp
@@ -58,6 +58,8 @@ int convert(Writer *writer, const FormatSection &to_conv) {
   case 'o':
   case 'x':
   case 'X':
+  case 'b':
+  case 'B':
     return convert_int(writer, to_conv);
 #ifndef LIBC_COPT_PRINTF_DISABLE_FLOAT
   case 'f':
diff --git a/libc/src/stdio/printf_core/int_converter.h b/libc/src/stdio/printf_core/int_converter.h
index 7744d801cbc189..8520f12c0372ce 100644
--- a/libc/src/stdio/printf_core/int_converter.h
+++ b/libc/src/stdio/printf_core/int_converter.h
@@ -33,6 +33,7 @@ using HexFmt = IntegerToString<uintmax_t, radix::Hex>;
 using HexFmtUppercase = IntegerToString<uintmax_t, radix::Hex::Uppercase>;
 using OctFmt = IntegerToString<uintmax_t, radix::Oct>;
 using DecFmt = IntegerToString<uintmax_t>;
+using BinFmt = IntegerToString<uintmax_t, radix::Bin>;
 
 LIBC_INLINE constexpr size_t num_buf_size() {
   constexpr auto max = [](size_t a, size_t b) -> size_t {
@@ -40,7 +41,8 @@ LIBC_INLINE constexpr size_t num_buf_size() {
   };
   return max(HexFmt::buffer_size(),
              max(HexFmtUppercase::buffer_size(),
-                 max(OctFmt::buffer_size(), DecFmt::buffer_size())));
+                 max(OctFmt::buffer_size(),
+                     max(DecFmt::buffer_size(), BinFmt::buffer_size()))));
 }
 
 LIBC_INLINE cpp::optional<cpp::string_view>
@@ -52,6 +54,8 @@ num_to_strview(uintmax_t num, cpp::span<char> bufref, char conv_name) {
       return HexFmtUppercase::format_to(bufref, num);
   } else if (conv_name == 'o') {
     return OctFmt::format_to(bufref, num);
+  } else if (to_lower(conv_name) == 'b') {
+    return BinFmt::format_to(bufref, num);
   } else {
     return DecFmt::format_to(bufref, num);
   }
diff --git a/libc/src/stdio/printf_core/parser.h b/libc/src/stdio/printf_core/parser.h
index ab491655275fb9..1e7d2e58c924a6 100644
--- a/libc/src/stdio/printf_core/parser.h
+++ b/libc/src/stdio/printf_core/parser.h
@@ -159,6 +159,8 @@ template <typename ArgProvider> class Parser {
       case ('x'):
       case ('X'):
       case ('u'):
+      case ('b'):
+      case ('B'):
         switch (lm) {
         case (LengthModifier::hh):
         case (LengthModifier::h):
@@ -484,6 +486,8 @@ template <typename ArgProvider> class Parser {
         case ('x'):
         case ('X'):
         case ('u'):
+        case ('b'):
+        case ('B'):
           switch (lm) {
           case (LengthModifier::hh):
           case (LengthModifier::h):
diff --git a/libc/test/src/stdio/printf_core/converter_test.cpp b/libc/test/src/stdio/printf_core/converter_test.cpp
index 8404ef6ec7db4d..9da749f3b8ad1a 100644
--- a/libc/test/src/stdio/printf_core/converter_test.cpp
+++ b/libc/test/src/stdio/printf_core/converter_test.cpp
@@ -210,6 +210,20 @@ TEST_F(LlvmLibcPrintfConverterTest, HexConversion) {
   ASSERT_EQ(writer.get_chars_written(), 18);
 }
 
+TEST_F(LlvmLibcPrintfConverterTest, BinaryConversion) {
+  LIBC_NAMESPACE::printf_core::FormatSection section;
+  section.has_conv = true;
+  section.raw_string = "%b";
+  section.conv_name = 'b';
+  section.conv_val_raw = 42;
+  LIBC_NAMESPACE::printf_core::convert(&writer, section);
+
+  wb.buff[wb.buff_cur] = '\0';
+
+  ASSERT_STREQ(str, "101010");
+  ASSERT_EQ(writer.get_chars_written(), 6);
+}
+
 TEST_F(LlvmLibcPrintfConverterTest, PointerConversion) {
 
   LIBC_NAMESPACE::printf_core::FormatSection section;

Copy link
Contributor

@michaelrj-google michaelrj-google left a comment

Choose a reason for hiding this comment

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

Overall this looks good, you do need to add support for the 0b prefix, which you can find in int_converter.h. Just search for prefix_len and you'll find it.

libc/src/stdio/printf_core/int_converter.h Outdated Show resolved Hide resolved
@agentcooper
Copy link
Contributor Author

@michaelrj-google I've updated the code according to your comments, could you please take another look?

Copy link
Contributor

@michaelrj-google michaelrj-google left a comment

Choose a reason for hiding this comment

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

LGTM with a nit. Feel free to land after fixing the nit, or ping me if you don't have commit access.

EXPECT_EQ(written, 6);
ASSERT_STREQ(buff, "101010");

// Precision Tests.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It would be good to have a test where the precision equals the number of digits, such as ("%3b", 0b111) -> "111"

@agentcooper
Copy link
Contributor Author

@michaelrj-google I've added a precision test. I don't have commit access, could you please merge this? And thanks a lot for the guidance!

@@ -533,6 +642,10 @@ TEST(LlvmLibcSPrintfTest, OctConv) {
EXPECT_EQ(written, 3);
ASSERT_STREQ(buff, "135");

written = LIBC_NAMESPACE::sprintf(buff, "%3b", 0b111);
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 put this in the wrong spot

Copy link
Contributor Author

@agentcooper agentcooper Feb 7, 2024

Choose a reason for hiding this comment

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

Ah, good catch. Made it early in the morning 😅 I've pushed an update.

@michaelrj-google
Copy link
Contributor

Thanks for creating this patch!

@michaelrj-google michaelrj-google merged commit e28ca2d into llvm:main Feb 7, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[libc] Support C23 binary notation in sprintf
3 participants