[libcxx-commits] [PATCH] D140653: [libc++][format] Implements range_formatter

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 6 inline comments as done.
Mordante added a comment.

Thanks for the review!



================
Comment at: libcxx/include/__format/buffer.h:518-519
+// __iterator of this container.
+template <__fmt_char_type _CharT>
+class _LIBCPP_TEMPLATE_VIS __retarget_buffer {
+public:
----------------
ldionne wrote:
> Would it be possible to use a `std::vector` internally instead of implementing this manually?
Yes, that's possible. It might have a small speed penalty, but since its usage is rare I think it's not too bad.


================
Comment at: libcxx/include/__format/range_formatter.h:223
+private:
+  _LIBCPP_HIDE_FROM_ABI constexpr void __parse_type(const _CharT*& __begin, const _CharT* __end) {
+    switch (*__begin) {
----------------
ldionne wrote:
> I think you're assuming that `string::iterator` is a `char*` here.
I assume `basic_string_view<_CharT>::const_iterator` is `_CharT*`. Which is currently true for libc++. I used to be convinced this was required by the Standard, but I recently learned this is not true.

I agree this should be fixed, but I prefer to do that in a different commit. In that commit I should fix all these assumptions. (All parsing code in format makes this assumption.)

I even wonder whether we should change `basic_string_view` to use a `wrap_iterator`.


================
Comment at: libcxx/test/std/utilities/format/format.range/format.range.formatter/set_brackets.pass.cpp:38
+
+  // Note there is no direct way to validate this function modified the object.
+}
----------------
ldionne wrote:
> Let's keep the comment, but  we can still add a simple test to validate that we're using the right brackets when we output a simple range.
I added a simple format test. Note `set_brackets` is `constexpr`, but `format` isn't (yet). So the additional validation is only done in the non-constexpr case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140653