[libc-commits] [PATCH] D129240: [libc] add printf hexadecimal float conversion

[Siva Chandra via Phabricator via libc-commits]

sivachandra accepted this revision.
sivachandra added a comment.
This revision is now accepted and ready to land.

Few nits inline.



================
Comment at: libc/src/stdio/printf_core/converter.cpp:47
     return convert_hex(writer, to_conv);
-  // TODO(michaelrj): add a flag to disable float point values here
+#ifndef LLVM_LIBC_PRINTF_DISABLE_FLOAT
   case 'f':
----------------
A comment for future: Do we have the CMake plumbing for any of these? If not, we should probably do it and have explicitly listed tests for them?


================
Comment at: libc/src/stdio/printf_core/float_hex_converter.h:103
+
+  static constexpr size_t BITS_IN_HEX_DIGIT = 4;
+
----------------
Why should this be static?


================
Comment at: libc/src/stdio/printf_core/float_hex_converter.h:116
+  // since the size must be constant.
+  static constexpr size_t MANT_BUFF_LEN =
+      (fputil::MantissaWidth<long double>::VALUE / BITS_IN_HEX_DIGIT) + 1;
----------------
Same here, why static?


================
Comment at: libc/src/stdio/printf_core/float_hex_converter.h:173
+  // 8  -> 3
+  static constexpr size_t EXP_LEN =
+      (((fputil::ExponentWidth<long double>::VALUE * 5) + 15) / 16) + 1;
----------------
Ditto.


================
Comment at: libc/src/stdio/printf_core/float_hex_converter.h:213
+                            FormatFlags::ALTERNATE_FORM);
+  constexpr char HEXADECIMAL_POINT = '.';
+
----------------
But is missing here!


================
Comment at: libc/src/stdio/printf_core/float_hex_converter.h:217
+  const char exp_seperator = a + ('p' - 'a');
+  constexpr int EXP_SEPERATOR_LEN = 1;
+
----------------
And here!


================
Comment at: libc/test/src/stdio/sprintf_test.cpp:20
+//    printf_func sprintf = ::sprintf;
+// }
+
----------------
What is this comment about?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129240