-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dynamic arguments storage. Implementation of enhancement from issue #1170. #1584
Conversation
Travis builds are fixed. Let's give a try with appveyor (don't know how to configure it for my fork yet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Please address the initial comments (inline).
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
Maybe worth adding
But I'd leave those for the a PR with named args. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. I left a few more comments, mostly minor/nits.
include/fmt/core.h
Outdated
#ifdef FMT_USE_STRING_VIEW | ||
template <typename Traits, typename Char> | ||
struct is_string_view<std::basic_string_view<Char, Traits>, Char> | ||
: std::true_type {}; | ||
#endif | ||
|
||
#ifdef FMT_USE_EXPERIMENTAL_STRING_VIEW | ||
template <typename Traits, typename Char> | ||
struct is_string_view<std::experimental::basic_string_view<Char, Traits>, Char> | ||
: std::true_type {}; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be replaced with
template <typename Char>
struct is_string_view<std_string_view<Char>, Char> : std::true_type {};
I wouldn't bother with traits at this time since they are not handled elsewhere in {fmt}.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I'd create an issue to track it then...
There's a constructor for internal::basic_string_view
from std::basic_string<>
with all the template parameters. But std_string_view
is recognized with only defaults...
include/fmt/core.h
Outdated
template <typename T> struct is_ref_wrapper : std::false_type {}; | ||
|
||
template <typename T> | ||
struct is_ref_wrapper<std::reference_wrapper<T>> : std::true_type {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to handle std::reference_wrapper
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, for me it's the main use-case :- ).
It's to let one to avoid dynamic memory allocation when the argument lifetime is guaranteed.
Actually I would want to make the dynamic_format_arg_store
reusable and more friendly for those who concerned about memory allocations:
- add reserve(); -- reserves
- add clean(); -- drops the
storage_
and resizes thedata_
to zero, but doesn't de-allocate thestorage_
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Thanks for clarification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also let's rename this trait to is_reference_wrapper
for consistency with the class name itself.
include/fmt/core.h
Outdated
struct is_ref_wrapper<std::reference_wrapper<T>> : std::true_type {}; | ||
|
||
template <typename T, typename Context> struct need_dyn_copy { | ||
using mapped_type = mapped_type_constant<T, Context>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you only use mapped_type::value
I suggest replacing this with
static internal::type mapped_type = mapped_type_constant<T, Context>::value;
and using mapped_type
in place of mapped_type::value
.
include/fmt/core.h
Outdated
template <typename T, typename Context> | ||
using need_dyn_copy_t = typename need_dyn_copy<T, Context>::type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's kill this alias since need_dyn_copy
is only used in one place.
include/fmt/core.h
Outdated
class dyn_arg_storage { | ||
// Workaround clang's -Wweak-vtables. For templates (unlike regular classes | ||
// doesn't complain about inability to deduce translation unit to place vtable | ||
// So dyn_arg_node_base is made a fake template |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean storage_node_base
instead of dyn_arg_node_base
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, thx
template <typename T, typename Context> | ||
using need_dyn_copy_t = typename need_dyn_copy<T, Context>::type; | ||
|
||
class dyn_arg_storage { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd merge this class into dynamic_format_arg_store
unless there is a strong reason to have it separate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
include/fmt/core.h
Outdated
using string_type = std::basic_string<char_type>; | ||
using value_type = basic_format_arg<Context>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd kill these two aliases since they are only used once each (and the notion of value_type is somewhat unclear here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
include/fmt/core.h
Outdated
// Storage of basic_format_arg must be contiguous | ||
// Required by basic_format_args::args_ which is just a pointer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: missing punctuation at the end of sentenses (.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
include/fmt/core.h
Outdated
template <typename T> const T& stored_value(const T& arg, std::false_type) { | ||
return arg; | ||
} | ||
|
||
template <typename T> | ||
const T& stored_value(const std::reference_wrapper<T>& arg, std::false_type) { | ||
return arg.get(); | ||
} | ||
|
||
template <typename T> | ||
const stored_type<T>& stored_value(const T& arg, std::true_type) { | ||
return storage_.push<stored_type<T>>(arg); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest replacing tag dispatch with a normal if in push_back
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
include/fmt/core.h
Outdated
static_assert( | ||
!std::is_base_of<internal::named_arg_base<char_type>, T>::value, | ||
"Named arguments are not supported yet"); | ||
emplace_arg(stored_value(arg, internal::need_dyn_copy_t<T, Context>{})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i.e. replacing this with something like
if (const_check(internal::need_dyn_copy_t<T, Context>()))
emplace_arg(storage_.push<stored_type<T>>(arg));
else
emplace_arg(arg);
You'll need to move const_check
from fmt/format.h
to fmt/core.h
. In the future this if
will become constexpr if
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Yes, let's leave these for a separate PR. |
`internal::std_string_veiew` alias
sorry, seems like I missed a few comments |
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.
Ok, seems to be done. Sorry for that false-start. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments (and there are several left over from previous review).
include/fmt/core.h
Outdated
template <typename T> struct is_ref_wrapper : std::false_type {}; | ||
|
||
template <typename T> | ||
struct is_ref_wrapper<std::reference_wrapper<T>> : std::true_type {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also let's rename this trait to is_reference_wrapper
for consistency with the class name itself.
include/fmt/core.h
Outdated
// Workaround clang's -Wweak-vtables. For templates (unlike regular classes | ||
// doesn't complain about inability to deduce translation unit to place vtable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had problems parsing the above. How about rephrasing to something like:
// Workaround clang's -Wweak-vtables for templates (unlike regular classes)
// don't complain about inability to deduce translation unit to place vtable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix.. Not exactly like you wrote but somehow... Hope it's better now
include/fmt/core.h
Outdated
Note that primitive type values are copied into basic_format_arg<>. | ||
\endrst |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just disallow passing reference wrappers of built-in types to prevent confusion.
// Unfortunately the tests are compiled with old ABI | ||
// So strings use COW. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imagine a std::string
with COW in shared state passed into dyn_arg_store by ref.
It takes fmt::string_view of the string. Next non-const subscription operator leads to COW, so string_view keeps pointing to an old version. So the test below won't work with old ABI std::string.
Merged, thanks! |
I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.
(Hope I didn't miss any guidelines.)