Support named args in dynamic_format_arg_store (#1655).#1663
Support named args in dynamic_format_arg_store (#1655).#1663vitaut merged 28 commits intofmtlib:masterfrom
Conversation
named_args will be added in a separate PR. Pending: avoid Args types from dynamic_format_arg_store template parameters, replace std::forward_list<std::variant> with a custom list.
Pending: cleanup.
Removed constexpr
Pending -- move dyn-args.h to core.h
`internal::std_string_veiew` alias
Changed dispatching from tag to explicit `if`. However I would prefer tag-based dispatching if `if constexpr`, but the later is not available in older standards.
Conflicts: include/fmt/core.h test/format-dyn-args-test.cc
vitaut
left a comment
vitaut
left a comment
There was a problem hiding this comment.
Thanks for the PR! Left a few preliminary comments inline and will take a look in details later.
|
@vitaut , could you take a look eventually? |
|
Will take a look shortly, sorry for slow response. |
vitaut
left a comment
vitaut
left a comment
There was a problem hiding this comment.
All looks good, just a few more nits.
|
|
||
| friend class basic_format_args<Context>; | ||
| friend class internal::arg_map<Context>; | ||
| friend class dynamic_format_arg_store<Context>; |
There was a problem hiding this comment.
Yes, basic_format_arg is constructed from dynamic_format_arg_store::emplace_arg() and needs to access private ctor. Also there's a need to update named_args with new size and pointer to array of named_arg_info after potential relocation.
| emplace_arg(dynamic_args_.push<stored_type<T>>(arg)); | ||
| else | ||
| emplace_arg(arg); | ||
| emplace_arg(static_cast<const internal::unwrap_reference_t<T>&>(arg)); |
There was a problem hiding this comment.
Same here. Then unwrap_reference_t can be removed.
| emplace_arg( | ||
| fmt::arg(arg_name, dynamic_args_.push<stored_type<T>>(arg.value))); | ||
| } else | ||
| emplace_arg(fmt::arg(arg_name, arg.value)); |
There was a problem hiding this comment.
Nit: please wrap the else condition in a compound statement ({}) for consistency.
|
Thank you, great work! |
2 participants
I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.