-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
fix: 'format_to_n' compiles 'std::back_inserter' arguments #913
fix: 'format_to_n' compiles 'std::back_inserter' arguments #913
Conversation
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.
Good catch, thanks.
truncating_iterator& operator++(int) { return *this; } | ||
|
||
truncating_iterator& operator*() { return *this; } | ||
}; |
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.
Is it possible to move common code between the two specializations into a superclass to reduce copy-paste?
58ab925
to
9dee8ab
Compare
Done ... |
include/fmt/format.h
Outdated
typedef typename OutputIt::container_type::value_type value_type; | ||
typedef void difference_type; | ||
typedef void pointer; | ||
typedef void reference; |
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 are these void
and are they different from their counterparts in iterator_traits
?
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.
void
actually is their type in iterator_traits
. We are talking about a back_insert_operator
here. I've basically just redefined the value_type
from void
to the value_type
of the underlying container.
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.
Then they can be moved to truncating_iterator_base
I think.
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 agree, even if it loses information about the underlying iterator type. Anyway, I've refactored these three void
typedefs into the common base class, the rest must stay as is: these are the essential differences between "regular output iterators" and "back insert interators".
std::back_insert_iterators model the OutputIterator concept but differ considerably in their traits and behavior. In particular the former made compilation to fail when format_to_n is given a back_inserter as first argument. The emulation of an OutputIterator is not perfect due to the behavioural differences of back_insert_iterators (e.g. assignment always implies increment) but good enough to be used within fmt's machinery.
9dee8ab
to
fc06d73
Compare
Merged, thanks! |
std::back_insert_iterators model the OutputIterator concept but differ considerably in their traits and behavior. In particular the former made compilation to fail when format_to_n is given a back_inserter as first argument. The emulation of an OutputIterator is not perfect due to the behavioural differences of back_insert_iterators (e.g. assignment always implies increment) but good enough to be used within fmt's machinery.