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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 4 inline comments as done.
Mordante added inline comments.


================
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.
----------------
ldionne wrote:
> 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.
Actually it I had already done the right thing(tm) but removed the comment and assertion.


================
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
----------------
ldionne wrote:
> 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.
I removed some. I still see some benefit since some syntax highlighting uses this information too. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129964