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

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius accepted this revision.
curdeius added a comment.

Apart from the question whether adding assertion fits here, this LGTM.



================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.parse.ctx/check_arg_id.pass.cpp:64
+  test();
+  test_exception();
+  static_assert(test());
----------------
Mordante wrote:
> curdeius wrote:
> > I don't see why you can't test the first part of `test_exception` in constexpr context. Changing return type to `bool` and returning false/true where appropriate should be enough to make it work.
> > Obviously, the part with `test_arg` can't be tested there.
> I'm not entirely sure what you mean. The line `context.next_arg_id();` will call `__format::__throw_error` which is not a `constexpr` function, turning it into one will fail with the following diagnostic ` error: constexpr function never produces a constant expression [-Winvalid-constexpr]`. Can you explain what change you think should work.
> 
> I made the separate function `test_exception` for the parts which can't be tested as `constexpr`.
You're completely right. I must have been confused when writing this:).


================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.parse.ctx/next_arg_id.pass.cpp:54
+  test();
+  test_exception();
+  static_assert(test());
----------------
curdeius wrote:
> As above, but here you can test everything in constexpr context.
Please ignore the above too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93166