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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 2 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/format:332
+      __indexing_ = __automatic;
+    return __next_arg_id_++;
+  }
----------------
curdeius wrote:
> I see that it isn't checked that `__next_arg_id_ < __num_args`.
> fmtlib has the following comment (https://github.com/fmtlib/fmt/blob/5a493560f59369e9fa664e8945b8e8a8ec4391b2/include/fmt/core.h#L599):
> ```
> // Don't check if the argument id is valid to avoid overhead and because it
> // will be checked during formatting anyway.
> ```
> 
> I don't know what's the libc++'s policy to use assertions, but for me it's a good place to put a `_LIBCPP_ASSERT`.
I didn't add a `_LIBCPP_ASSERT` since it indeed can be tested during formatting. If the index is out of bounds `basic_format_args::get` will return a default created `basic_format_arg` (http://eel.is/c++draft/format#args-4) This means the object has a `std::monostate` as value. In my WIP code I have a formatter for a `std::monostate` which will throw an exception.

I think it's better to throw an exception to inform the user about errors and I think it's not good to change that behaviour with debug macros. Note for other parts in my WIP code I use `_LIBCPP_ASSERT` to validate the expected state, but if they trigger it means the library code isn't robust enough.

@ldionne What do you think about the usage of `_LIBCPP_ASSERT` ?



================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.parse.ctx/check_arg_id.pass.cpp:64
+  test();
+  test_exception();
+  static_assert(test());
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93166