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

[Michael Jones via Phabricator via libc-commits]

michaelrj marked 6 inline comments as done.
michaelrj added inline comments.


================
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':
----------------
sivachandra wrote:
> 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?
There isn't CMake plumbing for these yet, I was planning on waiting until the main code was done to work on the cmake. The tests in `sprintf_test.cpp` are already set up to work when pieces are disabled by using the same flags, although I haven't added anything to test behavior when pieces are disabled.


================
Comment at: libc/src/stdio/printf_core/float_hex_converter.h:103
+
+  static constexpr size_t BITS_IN_HEX_DIGIT = 4;
+
----------------
sivachandra wrote:
> Why should this be static?
I don't remember, why I had some `constexpr` variables marked as static, so I've changed them all to not be static.


================
Comment at: libc/test/src/stdio/sprintf_test.cpp:20
+//    printf_func sprintf = ::sprintf;
+// }
+
----------------
sivachandra wrote:
> What is this comment about?
Ah, I forgot to remove that. I used it to make sure that my tests were correct by switching my implementation of printf with the system's.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129240