-
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
check for property 'mutable iterator' #921
check for property 'mutable iterator' #921
Conversation
please disregard so far, VS2017 passes the tests, but none of the compilers on Travis 😞 May be their library implementation is not SFINAE friendly when it comes to |
a9e3fa8
to
58d0416
Compare
The hostility towards SFINAE was indeed the killer! Now MSVC, gcc, and clang are happy campers again 😄 |
bdd8eb5
to
1943b5c
Compare
include/fmt/format.h
Outdated
template<typename... Ts> | ||
struct void_ { typedef void type; }; | ||
|
||
template <typename T, typename Enable = void_<>::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 void_<>::type
and not just void
? A comment might be good if this is required.
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 spotted, this is no longer necessary !
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.
Mostly looks good, but I have a few question inline.
include/fmt/format.h
Outdated
struct void_ { typedef void type; }; | ||
|
||
template <typename T, typename Enable = void_<>::type> | ||
struct it_category { typedef int 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.
Do I understand correctly that int
is arbitrary here and anything that is not std::output_iterator_tag
would do? Why not use void
? =)
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, void
cannot be used as an argument type to a function call. I've settled on std::false_type
now.
struct it_category<const T*> { typedef int type; }; | ||
|
||
template <typename T> | ||
struct it_category<T, typename void_<typename T::iterator_category>::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.
So this is basically a SFINAE detection of presence of T::iterator_category
? A comment would be nice as well.
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.
Yeah, I've added a comment to make this clear.
class is_output_iterator { | ||
//check for mutability | ||
template <typename U> | ||
static decltype(*(internal::declval<U>())) test(std::input_iterator_tag); |
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 not just return const char&
?
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.
Because dereferencing the iterator is the whole gist of all of this kaboodle. This returns either a const or a non-const refererence. The iterator traits
give no clue - in particular with regard to const_iterator
type members, and everything else fails on VS2013 as it's extra-finicky with expression-SFINAE.
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. I was confused by the use of input_iterator_tag
in the write context, but since iterator tags form a class hierarchy it makes sense now.
1943b5c
to
7b6e67d
Compare
Merged, thanks! |
constrain the
OutputIt
function parameters to be models ofmutable iterators
.