[libcxx-commits] [PATCH] D93166: [libc++][format] Add basic_format_parse_context.

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius requested changes to this revision.
curdeius added a comment.
This revision now requires changes to proceed.

You should mark tests that use `format_error` with `		// XFAIL: with_system_cxx_lib=macosx10.9|...|15`.



================
Comment at: libcxx/include/format:322
+  _LIBCPP_INLINE_VISIBILITY constexpr void advance_to(const_iterator __it) {
+    __begin_ = __it;
+  }
----------------
Couldn't we add at least a debug check to verify "Preconditions: end() is reachable from it."?
http://eel.is/c++draft/format#parse.ctx-5


================
Comment at: libcxx/include/format:342
+
+    if (std::is_constant_evaluated() && __id >= __num_args_)
+      __format::__throw_error("Argument index outside the valid range");
----------------
Why `is_constant_evaluated`? Does it correspond to the phrase "Remarks: Call expressions where id >= num_args_ are not core constant expressions ([expr.const])."?
http://eel.is/c++draft/format#parse.ctx-11
Could you explain how it should be understood?


================
Comment at: libcxx/include/format:349
+  iterator __end_;
+  enum _Indexing { __unknown, __manual, __automatic };
+  _Indexing __indexing_;
----------------
Why not enum class?


================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.parse.ctx/ctor.pass.cpp:33
+  static_assert(
+      !std::is_move_assignable_v<std::basic_format_parse_context<CharT> >);
+
----------------
Please add a test for the ctor being explicit (using `test_convertible` for instance).


================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.parse.ctx/types.compile.pass.cpp:47
+  test<wchar_t>();
+  test<char8_t>();
+  test<char16_t>();
----------------
I think you should guard it on `__cpp_char8_t`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93166