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

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius added inline comments.


================
Comment at: libcxx/include/format:332
+      __indexing_ = __automatic;
+    return __next_arg_id_++;
+  }
----------------
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`.


================
Comment at: libcxx/include/format:350
+    if (std::is_constant_evaluated() && __id >= __num_args_)
+      __format::__throw_error("Argument index outside the valid range");
+  }
----------------
Same as above, I'd expect at least an assertion in debug mode (in non-constexpr context).


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93166