[libcxx-commits] [PATCH] D129964: [libc++][format] Improve format buffer.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__format/buffer.h:97
+    // The same holds true for the fill.
+    // For transform it might be slightly harder, however the usecase for
+    // transform is slightly less common; it converts hexadecimal values to
----------------



================
Comment at: libcxx/include/__format/buffer.h:130-136
+    // The file test/std/utilities/format/format.functions/format_tests.h
+    // should have a test for the fallback algorithm. However the code is only
+    // used for integral hexadecimal. The floating point code does the
+    // transformation internally so never calls this code. That means even a
+    // 128-bit value will fit in capacity, making it not possible to test the
+    // code path.
+    // Alternatively only this assertion remains and the fallback code is removed.
----------------
I'm not super happy with this comment and this assertion. Either it is possible to hit that code path, in which case there should be a test for it and this comment can go. Or it's impossible to hit the code path, in which case the assertion stays (with a different diagnostic) but the comment goes.


================
Comment at: libcxx/include/__format/buffer.h:208
+  /// This approach works for all cases but one:
+  /// A \ref __format_to_n_buffer_base where \ref __enable_direct_output is
+  /// \c true. In that case the \ref __capacity_ of the buffer changes during
----------------
Side comment: I find it a bit hard to read with the Doxygen annotations. Since we don't generate Doxygen docs, I wonder what's the benefit of doing this.


================
Comment at: libcxx/include/__format/buffer.h:227
+  /// not the most common use case. Therefore the optimization isn't done.
+  _LIBCPP_HIDE_FROM_ABI void __flush_on_overflow(size_t __size) {
+    if (__size_ + __size >= __capacity_)
----------------
Where `__n` is the number of characters we want to write. Or maybe `__characters`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129964