[libcxx-commits] [PATCH] D93593: [libc++][format] Add __format_arg_store.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

Very small comments, leading presumably to a lot of changes in the next revision. I didn't review closely or anything.



================
Comment at: libcxx/include/format:365-370
+template <class _Visitor, class _Context>
+_LIBCPP_INLINE_VISIBILITY auto
+visit_format_arg(_Visitor&& __vis, basic_format_arg<_Context> __arg)
+    -> decltype(_VSTD::visit(forward<_Visitor>(__vis), __arg.__value_)) {
+  return _VSTD::visit(forward<_Visitor>(__vis), __arg.__value_);
+}
----------------
Nit throughout: `_VSTD::forward` (and all other functions of >=1 argument) because ADL.
But you //don't// need (and therefore shouldn't put) `_VSTD::` on concepts, classes, or variables; i.e. you should write `monostate` not `_VSTD::monostate`, and `same_as` not `_VSTD::same_as`, and so on.
Essentially, just as you should think of `explicit` as meaning "Here comes a constructor!", you should think of `_VSTD::` in the libc++ codebase as meaning "Here comes a function call!"

Hopefully this fixes line 442's `)&&sizeof(_Tp)` misformatting, as well.


================
Comment at: libcxx/include/format:485-486
+  explicit basic_format_arg(const char_type* __s) {
+    if (!__s)
+      __throw_format_error("Used a nullptr argument to initialize a C-string.");
+    __value_ = __s;
----------------
This being a "Precondition", I think it'd be better to use `_LIBCPP_ASSERT` here. Users of libc++ shouldn't be taught to rely on this exception happening, because it'll probably stop happening as soon (heh) as Contracts lands.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93593