[libcxx-commits] [PATCH] D129964: [libc++][format] Improve format buffer.
Mark de Wever via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Sat Aug 13 02:44:10 PDT 2022
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
More information about the libcxx-commits
mailing list