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

[Mark de Wever via Phabricator via libcxx-commits]

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

Thanks for the review!

I'll add the XFAILs to the next version of the patch.



================
Comment at: libcxx/include/format:322
+  _LIBCPP_INLINE_VISIBILITY constexpr void advance_to(const_iterator __it) {
+    __begin_ = __it;
+  }
----------------
curdeius wrote:
> 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
I had a look at the debug iterators and adding this test seems not trivial. So I think it's not worth the effort to add it here.


================
Comment at: libcxx/include/format:342
+
+    if (std::is_constant_evaluated() && __id >= __num_args_)
+      __format::__throw_error("Argument index outside the valid range");
----------------
curdeius wrote:
> 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?
Since http://eel.is/c++draft/format#parse.ctx-10 doesn't specify this exception to be thrown I read this that the test only should be done when `std::is_constant_evaluated` is `true`. In that case the  function should no longer be a `constexpr` function. So I use the throw to achieve this. I'm not sure why the exception just shouldn't be thrown unconditionally, I hope to figure that out when I'm a bit further with the implementation.

I'll add some comment in the next revision of the patch.


================
Comment at: libcxx/include/format:349
+  iterator __end_;
+  enum _Indexing { __unknown, __manual, __automatic };
+  _Indexing __indexing_;
----------------
curdeius wrote:
> Why not enum class?
I didn't add it since the enum is only used internally in the class so I feel the `class` doesn't add to much benefits and http://eel.is/c++draft/format.parse.ctx doesn't require it.

If you feel strongly about it I don't mind changing it.


================
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>();
----------------
curdeius wrote:
> I think you should guard it on `__cpp_char8_t`.
I thought it wouldn't be required, but I'll add `_LIBCPP_NO_HAS_CHAR8_T` and `_LIBCPP_HAS_NO_UNICODE_CHARS` guards.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93166