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

[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:502
+
+#  if _LIBCPP_STD_VER > 20
+// A dynamically growing buffer intended to be used for retargeting a context.
----------------
I wouldn't guard this as being >= C++23, since this is an implementation detail. I would only guard the range formatter itself.

Same applies below.


================
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:
----------------
Would it be possible to use a `std::vector` internally instead of implementing this manually?


================
Comment at: libcxx/test/std/utilities/format/format.range/format.range.formatter/format.functions.format.pass.cpp:1
+//===----------------------------------------------------------------------===//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
----------------
Since this is a general range formatter, I would like to see tests for formatting different kinds of ranges, like an input range, forward range, etc.


================
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.
+}
----------------
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.


================
Comment at: libcxx/test/std/utilities/format/format.range/format.range.formatter/set_separator.pass.cpp:37
+
+  // Note there is no direct way to validate this function modified the object.
+}
----------------
Same comment as for brackets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140653