A C++ custom iterator for indirection iteration
Clash Royale CLAN TAG#URR8PPP
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;
up vote
4
down vote
favorite
This program does not quite conform to the previous related question, yet does basically the same thing: given a collection of elements and a collection of indices, return via an iterator collection elements according to the sequence of indices. Here it goes:
#include <exception>
#include <iostream>
#include <iterator>
#include <list>
#include <sstream>
#include <vector>
template<typename ForwardIterator, typename IndexIterator>
class indirection_iterator
public:
indirection_iterator(ForwardIterator element_begin,
ForwardIterator element_end,
IndexIterator index_begin,
IndexIterator index_end)
:
m_element_beginelement_begin,
m_element_endelement_end,
m_index_beginindex_begin,
m_index_endindex_end,
m_index_currentindex_begin,
m_elementsstd::distance(element_begin, element_end)
indirection_iterator begin()
return indirection_iteratorm_element_begin,
m_element_end,
m_index_begin,
m_index_end,
m_index_begin;
indirection_iterator end()
return indirection_iteratorm_element_begin,
m_element_end,
m_index_begin,
m_index_end,
m_index_end;
auto operator*()
auto index = *m_index_current;
check_index(index, m_elements);
auto element_iterator = m_element_begin;
std::advance(element_iterator, index);
return *element_iterator;
void operator++()
std::advance(m_index_current, 1);
bool operator!=(indirection_iterator const& other)
return m_index_current != other.m_index_current;
private:
indirection_iterator(ForwardIterator element_begin,
ForwardIterator element_end,
IndexIterator index_begin,
IndexIterator index_end,
IndexIterator index_current)
:
m_element_beginelement_begin,
m_element_endelement_end,
m_index_beginindex_begin,
m_index_endindex_end,
m_index_currentindex_current,
m_elementsstd::distance(element_begin, element_end)
template<typename IndexIter>
using index_type = typename std::iterator_traits<IndexIter>::value_type;
ForwardIterator m_element_begin;
ForwardIterator m_element_end;
IndexIterator m_index_begin;
IndexIterator m_index_end;
IndexIterator m_index_current;
typename std::iterator_traits<ForwardIterator>::difference_type m_elements;
template<typename index_type>
void check_index(index_type index, size_t elements)
if (index < 0)
std::stringstream ss;
ss << "index(" << index << ") < 0";
throw std::runtime_errorss.str();
if (index >= elements)
std::stringstream ss;
ss << "index(" << index << ") >= elements(" << elements << ")";
throw std::runtime_errorss.str();
;
static std::vector<char> get_alphabet()
std::vector<char> alphabet;
for (char ch = 'a'; ch <= 'z'; ch++)
alphabet.push_back(ch);
for (char ch = 'A'; ch <= 'Z'; ch++)
alphabet.push_back(ch);
alphabet.push_back(',');
alphabet.push_back(' ');
alphabet.push_back('!');
alphabet.push_back('n');
return alphabet;
int main(int argc, const char * argv)
std::vector<char> alphabet = get_alphabet();
std::list<int> char_indices =
33, 4, 11, 11, 14, 52, 53, 48, 14, 17, 11, 3, 54, 55 ;
indirection_iterator<decltype(alphabet.cbegin()),
decltype(char_indices.cbegin())> indirection_iter
alphabet.cbegin(),
alphabet.cend(),
char_indices.cbegin(),
char_indices.cend()
;
std::copy(indirection_iter.begin(),
indirection_iter.end(),
std::ostream_iterator<char>std::cout);
return 0;
Critique request
I would like to hear how to make it more usable and how to make it more idiomatic C++.
c++ iterator
add a comment |Â
up vote
4
down vote
favorite
This program does not quite conform to the previous related question, yet does basically the same thing: given a collection of elements and a collection of indices, return via an iterator collection elements according to the sequence of indices. Here it goes:
#include <exception>
#include <iostream>
#include <iterator>
#include <list>
#include <sstream>
#include <vector>
template<typename ForwardIterator, typename IndexIterator>
class indirection_iterator
public:
indirection_iterator(ForwardIterator element_begin,
ForwardIterator element_end,
IndexIterator index_begin,
IndexIterator index_end)
:
m_element_beginelement_begin,
m_element_endelement_end,
m_index_beginindex_begin,
m_index_endindex_end,
m_index_currentindex_begin,
m_elementsstd::distance(element_begin, element_end)
indirection_iterator begin()
return indirection_iteratorm_element_begin,
m_element_end,
m_index_begin,
m_index_end,
m_index_begin;
indirection_iterator end()
return indirection_iteratorm_element_begin,
m_element_end,
m_index_begin,
m_index_end,
m_index_end;
auto operator*()
auto index = *m_index_current;
check_index(index, m_elements);
auto element_iterator = m_element_begin;
std::advance(element_iterator, index);
return *element_iterator;
void operator++()
std::advance(m_index_current, 1);
bool operator!=(indirection_iterator const& other)
return m_index_current != other.m_index_current;
private:
indirection_iterator(ForwardIterator element_begin,
ForwardIterator element_end,
IndexIterator index_begin,
IndexIterator index_end,
IndexIterator index_current)
:
m_element_beginelement_begin,
m_element_endelement_end,
m_index_beginindex_begin,
m_index_endindex_end,
m_index_currentindex_current,
m_elementsstd::distance(element_begin, element_end)
template<typename IndexIter>
using index_type = typename std::iterator_traits<IndexIter>::value_type;
ForwardIterator m_element_begin;
ForwardIterator m_element_end;
IndexIterator m_index_begin;
IndexIterator m_index_end;
IndexIterator m_index_current;
typename std::iterator_traits<ForwardIterator>::difference_type m_elements;
template<typename index_type>
void check_index(index_type index, size_t elements)
if (index < 0)
std::stringstream ss;
ss << "index(" << index << ") < 0";
throw std::runtime_errorss.str();
if (index >= elements)
std::stringstream ss;
ss << "index(" << index << ") >= elements(" << elements << ")";
throw std::runtime_errorss.str();
;
static std::vector<char> get_alphabet()
std::vector<char> alphabet;
for (char ch = 'a'; ch <= 'z'; ch++)
alphabet.push_back(ch);
for (char ch = 'A'; ch <= 'Z'; ch++)
alphabet.push_back(ch);
alphabet.push_back(',');
alphabet.push_back(' ');
alphabet.push_back('!');
alphabet.push_back('n');
return alphabet;
int main(int argc, const char * argv)
std::vector<char> alphabet = get_alphabet();
std::list<int> char_indices =
33, 4, 11, 11, 14, 52, 53, 48, 14, 17, 11, 3, 54, 55 ;
indirection_iterator<decltype(alphabet.cbegin()),
decltype(char_indices.cbegin())> indirection_iter
alphabet.cbegin(),
alphabet.cend(),
char_indices.cbegin(),
char_indices.cend()
;
std::copy(indirection_iter.begin(),
indirection_iter.end(),
std::ostream_iterator<char>std::cout);
return 0;
Critique request
I would like to hear how to make it more usable and how to make it more idiomatic C++.
c++ iterator
1
Does not compile. I think it's because the iterator traits are supposed to be discoverable by (standard) algorithms, so you can't make themprivate
.
â nwp
Jan 17 at 20:23
@nwp Did on Xcode yet I had to set C++14 in the settings. I will take a look at the issue but not today.
â coderodde
Jan 17 at 20:26
add a comment |Â
up vote
4
down vote
favorite
up vote
4
down vote
favorite
This program does not quite conform to the previous related question, yet does basically the same thing: given a collection of elements and a collection of indices, return via an iterator collection elements according to the sequence of indices. Here it goes:
#include <exception>
#include <iostream>
#include <iterator>
#include <list>
#include <sstream>
#include <vector>
template<typename ForwardIterator, typename IndexIterator>
class indirection_iterator
public:
indirection_iterator(ForwardIterator element_begin,
ForwardIterator element_end,
IndexIterator index_begin,
IndexIterator index_end)
:
m_element_beginelement_begin,
m_element_endelement_end,
m_index_beginindex_begin,
m_index_endindex_end,
m_index_currentindex_begin,
m_elementsstd::distance(element_begin, element_end)
indirection_iterator begin()
return indirection_iteratorm_element_begin,
m_element_end,
m_index_begin,
m_index_end,
m_index_begin;
indirection_iterator end()
return indirection_iteratorm_element_begin,
m_element_end,
m_index_begin,
m_index_end,
m_index_end;
auto operator*()
auto index = *m_index_current;
check_index(index, m_elements);
auto element_iterator = m_element_begin;
std::advance(element_iterator, index);
return *element_iterator;
void operator++()
std::advance(m_index_current, 1);
bool operator!=(indirection_iterator const& other)
return m_index_current != other.m_index_current;
private:
indirection_iterator(ForwardIterator element_begin,
ForwardIterator element_end,
IndexIterator index_begin,
IndexIterator index_end,
IndexIterator index_current)
:
m_element_beginelement_begin,
m_element_endelement_end,
m_index_beginindex_begin,
m_index_endindex_end,
m_index_currentindex_current,
m_elementsstd::distance(element_begin, element_end)
template<typename IndexIter>
using index_type = typename std::iterator_traits<IndexIter>::value_type;
ForwardIterator m_element_begin;
ForwardIterator m_element_end;
IndexIterator m_index_begin;
IndexIterator m_index_end;
IndexIterator m_index_current;
typename std::iterator_traits<ForwardIterator>::difference_type m_elements;
template<typename index_type>
void check_index(index_type index, size_t elements)
if (index < 0)
std::stringstream ss;
ss << "index(" << index << ") < 0";
throw std::runtime_errorss.str();
if (index >= elements)
std::stringstream ss;
ss << "index(" << index << ") >= elements(" << elements << ")";
throw std::runtime_errorss.str();
;
static std::vector<char> get_alphabet()
std::vector<char> alphabet;
for (char ch = 'a'; ch <= 'z'; ch++)
alphabet.push_back(ch);
for (char ch = 'A'; ch <= 'Z'; ch++)
alphabet.push_back(ch);
alphabet.push_back(',');
alphabet.push_back(' ');
alphabet.push_back('!');
alphabet.push_back('n');
return alphabet;
int main(int argc, const char * argv)
std::vector<char> alphabet = get_alphabet();
std::list<int> char_indices =
33, 4, 11, 11, 14, 52, 53, 48, 14, 17, 11, 3, 54, 55 ;
indirection_iterator<decltype(alphabet.cbegin()),
decltype(char_indices.cbegin())> indirection_iter
alphabet.cbegin(),
alphabet.cend(),
char_indices.cbegin(),
char_indices.cend()
;
std::copy(indirection_iter.begin(),
indirection_iter.end(),
std::ostream_iterator<char>std::cout);
return 0;
Critique request
I would like to hear how to make it more usable and how to make it more idiomatic C++.
c++ iterator
This program does not quite conform to the previous related question, yet does basically the same thing: given a collection of elements and a collection of indices, return via an iterator collection elements according to the sequence of indices. Here it goes:
#include <exception>
#include <iostream>
#include <iterator>
#include <list>
#include <sstream>
#include <vector>
template<typename ForwardIterator, typename IndexIterator>
class indirection_iterator
public:
indirection_iterator(ForwardIterator element_begin,
ForwardIterator element_end,
IndexIterator index_begin,
IndexIterator index_end)
:
m_element_beginelement_begin,
m_element_endelement_end,
m_index_beginindex_begin,
m_index_endindex_end,
m_index_currentindex_begin,
m_elementsstd::distance(element_begin, element_end)
indirection_iterator begin()
return indirection_iteratorm_element_begin,
m_element_end,
m_index_begin,
m_index_end,
m_index_begin;
indirection_iterator end()
return indirection_iteratorm_element_begin,
m_element_end,
m_index_begin,
m_index_end,
m_index_end;
auto operator*()
auto index = *m_index_current;
check_index(index, m_elements);
auto element_iterator = m_element_begin;
std::advance(element_iterator, index);
return *element_iterator;
void operator++()
std::advance(m_index_current, 1);
bool operator!=(indirection_iterator const& other)
return m_index_current != other.m_index_current;
private:
indirection_iterator(ForwardIterator element_begin,
ForwardIterator element_end,
IndexIterator index_begin,
IndexIterator index_end,
IndexIterator index_current)
:
m_element_beginelement_begin,
m_element_endelement_end,
m_index_beginindex_begin,
m_index_endindex_end,
m_index_currentindex_current,
m_elementsstd::distance(element_begin, element_end)
template<typename IndexIter>
using index_type = typename std::iterator_traits<IndexIter>::value_type;
ForwardIterator m_element_begin;
ForwardIterator m_element_end;
IndexIterator m_index_begin;
IndexIterator m_index_end;
IndexIterator m_index_current;
typename std::iterator_traits<ForwardIterator>::difference_type m_elements;
template<typename index_type>
void check_index(index_type index, size_t elements)
if (index < 0)
std::stringstream ss;
ss << "index(" << index << ") < 0";
throw std::runtime_errorss.str();
if (index >= elements)
std::stringstream ss;
ss << "index(" << index << ") >= elements(" << elements << ")";
throw std::runtime_errorss.str();
;
static std::vector<char> get_alphabet()
std::vector<char> alphabet;
for (char ch = 'a'; ch <= 'z'; ch++)
alphabet.push_back(ch);
for (char ch = 'A'; ch <= 'Z'; ch++)
alphabet.push_back(ch);
alphabet.push_back(',');
alphabet.push_back(' ');
alphabet.push_back('!');
alphabet.push_back('n');
return alphabet;
int main(int argc, const char * argv)
std::vector<char> alphabet = get_alphabet();
std::list<int> char_indices =
33, 4, 11, 11, 14, 52, 53, 48, 14, 17, 11, 3, 54, 55 ;
indirection_iterator<decltype(alphabet.cbegin()),
decltype(char_indices.cbegin())> indirection_iter
alphabet.cbegin(),
alphabet.cend(),
char_indices.cbegin(),
char_indices.cend()
;
std::copy(indirection_iter.begin(),
indirection_iter.end(),
std::ostream_iterator<char>std::cout);
return 0;
Critique request
I would like to hear how to make it more usable and how to make it more idiomatic C++.
c++ iterator
asked Jan 17 at 20:20
coderodde
15.5k533114
15.5k533114
1
Does not compile. I think it's because the iterator traits are supposed to be discoverable by (standard) algorithms, so you can't make themprivate
.
â nwp
Jan 17 at 20:23
@nwp Did on Xcode yet I had to set C++14 in the settings. I will take a look at the issue but not today.
â coderodde
Jan 17 at 20:26
add a comment |Â
1
Does not compile. I think it's because the iterator traits are supposed to be discoverable by (standard) algorithms, so you can't make themprivate
.
â nwp
Jan 17 at 20:23
@nwp Did on Xcode yet I had to set C++14 in the settings. I will take a look at the issue but not today.
â coderodde
Jan 17 at 20:26
1
1
Does not compile. I think it's because the iterator traits are supposed to be discoverable by (standard) algorithms, so you can't make them
private
.â nwp
Jan 17 at 20:23
Does not compile. I think it's because the iterator traits are supposed to be discoverable by (standard) algorithms, so you can't make them
private
.â nwp
Jan 17 at 20:23
@nwp Did on Xcode yet I had to set C++14 in the settings. I will take a look at the issue but not today.
â coderodde
Jan 17 at 20:26
@nwp Did on Xcode yet I had to set C++14 in the settings. I will take a look at the issue but not today.
â coderodde
Jan 17 at 20:26
add a comment |Â
2 Answers
2
active
oldest
votes
up vote
6
down vote
accepted
You are missing the iterator_traits
types
If there were only one big tip I could give you, it would be to add the required type aliases for compatibility with the standard iterator interface. Citing from working draft N 4296 of the C++ standard (the last working draft before C++14), section 24.4.1 [iterator.traits]:
To implement algorithms only in terms of iterators, it is often
necessary to determine the value and difference types that correspond
to a particular iterator type. Accordingly, it is required that if
Iterator is the type of an iterator, the typesiterator_traits<Iterator>::difference_type
iterator_traits<Iterator>::value_type
iterator_traits<Iterator>::iterator_category
be defined as the
iteratorâÂÂs difference type, value type and iterator category,
respectively. In addition, the typesiterator_traits<Iterator>::reference
iterator_traits<Iterator>::pointer
shall be defined as the iteratorâÂÂs
reference and pointer types, that is, for an iterator objecta
, the
same type as the type of*a
anda->
, respectively. In the case of an
output iterator, the typesiterator_traits<Iterator>::difference_type
iterator_traits<Iterator>::value_type
iterator_traits<Iterator>::reference
iterator_traits<Iterator>::pointer
may be defined as
void
.
Thus, your implementation is currently not a valid iterator in the STL sense (which was also already noticed by nwp in the comments).
Your interface is a little confusing
The class that you wrote, indirection_iterator
, does not only serve as an iterator, but also as a kind of iterator factory. Code such as
indirection_iterator<decltype(alphabet.cbegin()),
decltype(char_indices.cbegin())> indirection_iter
alphabet.cbegin(),
alphabet.cend(),
char_indices.cbegin(),
char_indices.cend()
;
std::copy(indirection_iter.begin(),
indirection_iter.end(),
std::ostream_iterator<char>std::cout);
makes me feel really uneasy because it goes against how iterators are used by the STL. Calling begin
and end
on an iterator just feels wrong.
I suggest splitting up the functionality into two classes instead: One class which is used to make indirection iterators from a given set of value and index iterators (called indirection_iterator_factory
, for example) and an indirection_iterator
class which encapsulates only the iterator functionality.
De-cluttering
An added benefit of this method would be that you could reduce your actual iterator class by a few members. Another point which makes me uneasy is the fact that you have six different members in what is basically a simple iterator class, some of which are unnecessary as far as I can see. For example, you never make use of m_index_begin
, so that should be the first variable to go. The same is true for m_index_end
, as for m_element_end
.
Before we go on removing stuff, let's talk a bit about iterator design here. In particular, I want to talk about check_index
. So far, you are evaluating whether a particular index is in bounds for every access and, if it is not, throw an exception. That last part is going to make some people angry, because throwing exceptions is something a not negligible part of C++ developers disapprove of, and on some platforms, exceptions are not even usable at all. This does not mean that you should never rely on exceptions, but in my opinion, you should make everything relying on exceptions opt-in.
In your case, that basically means extracting the index check into a public interface method that the user can call if he wants, but doesn't have to. This is akin to how this issue is usually dealt with in the standard library: For example, std::vector
offers both an unchecked operator
as well as a checked (and exception-throwing) method at
.
Having said that, I do actually believe that iterators should be as minimalistic as possible and only offer what they were designed to do, which is, in most cases, iterating a certain range of objects. If I had to rewrite your indirection_iterator
, I would certainly leave out range checking as a whole, and require that all the indices in the underlying container be valid (again, it does seem to me that this largely reflects how the iterators in the STL were designed). This is a matter of opinion, however, so don't take this as a must-do.
Adding to the interface
The interface of indirection_iterator
is currently kind of meager. For example, operator->
is missing, which is a requirement for input iterators; an iterator category you could easily fulfill. The same is true for pre-increment, and having operator==
, although not strictly required, would probably be reasonable. Another thing which I would definitely add is a way to retrieve the current index, since this opens up a wide variety of possible use cases for your users, ranging from logging elements with their index to doing bounds checking manually.
Then there is a whole group of methods which are difficult to provide, but oftentimes very reasonable, e.g. operator--
. The difficulty here arises from the fact that what you have written is less of an actual iterator and more of an iterator adapter.
Optimizing functionality for different iterators could definitely be beneficial to usability as well as speed, but the big downside is that it will be a lot of work to implement. Ultimately, it depends on your use case, but if you really wanted to make this iterator part of a publicly available library, you should do that step. Otherwise, it is purely up to you whether you want to invest that much time and effort.
Little things and nitpicks
- The name of the type parameter to
check_index
,index_type
, is the same as an alias template you defined before. This could possibly lead to name shadowing or worse, so I would suggest you change either of those two names. - The template typename parameter
IndexIterator
does not convey what type of iterator you require. I would suggestIndexForwardIterator
. operator!=
is currently somewhat dysfunctional, the reason being that you only check for index equality, but not whether both of the iterators are iterators over the same container. Thus, if I had an iterator over container a with indices i and an iterator over container b with indices i, both iterators could still compare equal, which is likely not what you want.
IâÂÂm not sure about calling begin on iterator being wrong. May be you expected dereference first?
â Incomputable
Jan 18 at 6:46
1
@Incomputable No, this is more of a general idea of iterator design. An iterator is basically a generalization of a pointer, and is usually obtained from a container by callingbegin
andend
. Callingbegin
andend
on the iterator itself doesn't make much sense from a design point of view, because you are not iterating over the iterator but the underlying container(s) instead.
â Ben Steffan
Jan 18 at 14:36
Nice one! BTW, I am pretty sure that I can deal with basic C++ syntax, but would it be a good idea to read a standard draft as a "course book"?
â coderodde
Apr 29 at 8:52
1
@coderodde If you have the dexterity to read a whole draft, then there is probably a lot you can learn from that. I certainly don't, but in any case, I keep a standard draft around just to look up certain things from time to time. It's probably the most invaluable resource for C++ programmers, once you have figured out how to read the somewhat convoluted "standardese".
â Ben Steffan
Apr 29 at 8:56
add a comment |Â
up vote
3
down vote
A portability problem
I know this is just the test code, rather than the real subject of the review, but it's worth mentioning.
for (char ch = 'a'; ch <= 'z'; ch++)
alphabet.push_back(ch);
This doesn't necessarily do what you think it does; it depends on the target's character coding. In EBCDIC locales, this will insert 41 values, which will throw your indexing off.
It's safer and clearer to initialize from a string literal:
static std::vector<char> get_alphabet()
static char const s = ""
"abcdefghijklmnopqrstuvwxyz"
"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
", !n";
return std::begin(s), std::end(s);
Iterator interface
GCC warns (with -Weffc++
) that operator++()
should conventionally return a reference to *this
, not void
. Many of the methods could/should be declared const
, too (at least begin
, end
, operator*
, operator!=
and check_index
, per my testing).
User Incomputable recommended that I point you at operator overloading (on cppreference.com) as a useful list of the usual function signatures.
BTW, if anyone is wondering - that initial""
is there just to help Emacs line up the rows ofs
.
â Toby Speight
Jan 18 at 14:15
add a comment |Â
2 Answers
2
active
oldest
votes
2 Answers
2
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
6
down vote
accepted
You are missing the iterator_traits
types
If there were only one big tip I could give you, it would be to add the required type aliases for compatibility with the standard iterator interface. Citing from working draft N 4296 of the C++ standard (the last working draft before C++14), section 24.4.1 [iterator.traits]:
To implement algorithms only in terms of iterators, it is often
necessary to determine the value and difference types that correspond
to a particular iterator type. Accordingly, it is required that if
Iterator is the type of an iterator, the typesiterator_traits<Iterator>::difference_type
iterator_traits<Iterator>::value_type
iterator_traits<Iterator>::iterator_category
be defined as the
iteratorâÂÂs difference type, value type and iterator category,
respectively. In addition, the typesiterator_traits<Iterator>::reference
iterator_traits<Iterator>::pointer
shall be defined as the iteratorâÂÂs
reference and pointer types, that is, for an iterator objecta
, the
same type as the type of*a
anda->
, respectively. In the case of an
output iterator, the typesiterator_traits<Iterator>::difference_type
iterator_traits<Iterator>::value_type
iterator_traits<Iterator>::reference
iterator_traits<Iterator>::pointer
may be defined as
void
.
Thus, your implementation is currently not a valid iterator in the STL sense (which was also already noticed by nwp in the comments).
Your interface is a little confusing
The class that you wrote, indirection_iterator
, does not only serve as an iterator, but also as a kind of iterator factory. Code such as
indirection_iterator<decltype(alphabet.cbegin()),
decltype(char_indices.cbegin())> indirection_iter
alphabet.cbegin(),
alphabet.cend(),
char_indices.cbegin(),
char_indices.cend()
;
std::copy(indirection_iter.begin(),
indirection_iter.end(),
std::ostream_iterator<char>std::cout);
makes me feel really uneasy because it goes against how iterators are used by the STL. Calling begin
and end
on an iterator just feels wrong.
I suggest splitting up the functionality into two classes instead: One class which is used to make indirection iterators from a given set of value and index iterators (called indirection_iterator_factory
, for example) and an indirection_iterator
class which encapsulates only the iterator functionality.
De-cluttering
An added benefit of this method would be that you could reduce your actual iterator class by a few members. Another point which makes me uneasy is the fact that you have six different members in what is basically a simple iterator class, some of which are unnecessary as far as I can see. For example, you never make use of m_index_begin
, so that should be the first variable to go. The same is true for m_index_end
, as for m_element_end
.
Before we go on removing stuff, let's talk a bit about iterator design here. In particular, I want to talk about check_index
. So far, you are evaluating whether a particular index is in bounds for every access and, if it is not, throw an exception. That last part is going to make some people angry, because throwing exceptions is something a not negligible part of C++ developers disapprove of, and on some platforms, exceptions are not even usable at all. This does not mean that you should never rely on exceptions, but in my opinion, you should make everything relying on exceptions opt-in.
In your case, that basically means extracting the index check into a public interface method that the user can call if he wants, but doesn't have to. This is akin to how this issue is usually dealt with in the standard library: For example, std::vector
offers both an unchecked operator
as well as a checked (and exception-throwing) method at
.
Having said that, I do actually believe that iterators should be as minimalistic as possible and only offer what they were designed to do, which is, in most cases, iterating a certain range of objects. If I had to rewrite your indirection_iterator
, I would certainly leave out range checking as a whole, and require that all the indices in the underlying container be valid (again, it does seem to me that this largely reflects how the iterators in the STL were designed). This is a matter of opinion, however, so don't take this as a must-do.
Adding to the interface
The interface of indirection_iterator
is currently kind of meager. For example, operator->
is missing, which is a requirement for input iterators; an iterator category you could easily fulfill. The same is true for pre-increment, and having operator==
, although not strictly required, would probably be reasonable. Another thing which I would definitely add is a way to retrieve the current index, since this opens up a wide variety of possible use cases for your users, ranging from logging elements with their index to doing bounds checking manually.
Then there is a whole group of methods which are difficult to provide, but oftentimes very reasonable, e.g. operator--
. The difficulty here arises from the fact that what you have written is less of an actual iterator and more of an iterator adapter.
Optimizing functionality for different iterators could definitely be beneficial to usability as well as speed, but the big downside is that it will be a lot of work to implement. Ultimately, it depends on your use case, but if you really wanted to make this iterator part of a publicly available library, you should do that step. Otherwise, it is purely up to you whether you want to invest that much time and effort.
Little things and nitpicks
- The name of the type parameter to
check_index
,index_type
, is the same as an alias template you defined before. This could possibly lead to name shadowing or worse, so I would suggest you change either of those two names. - The template typename parameter
IndexIterator
does not convey what type of iterator you require. I would suggestIndexForwardIterator
. operator!=
is currently somewhat dysfunctional, the reason being that you only check for index equality, but not whether both of the iterators are iterators over the same container. Thus, if I had an iterator over container a with indices i and an iterator over container b with indices i, both iterators could still compare equal, which is likely not what you want.
IâÂÂm not sure about calling begin on iterator being wrong. May be you expected dereference first?
â Incomputable
Jan 18 at 6:46
1
@Incomputable No, this is more of a general idea of iterator design. An iterator is basically a generalization of a pointer, and is usually obtained from a container by callingbegin
andend
. Callingbegin
andend
on the iterator itself doesn't make much sense from a design point of view, because you are not iterating over the iterator but the underlying container(s) instead.
â Ben Steffan
Jan 18 at 14:36
Nice one! BTW, I am pretty sure that I can deal with basic C++ syntax, but would it be a good idea to read a standard draft as a "course book"?
â coderodde
Apr 29 at 8:52
1
@coderodde If you have the dexterity to read a whole draft, then there is probably a lot you can learn from that. I certainly don't, but in any case, I keep a standard draft around just to look up certain things from time to time. It's probably the most invaluable resource for C++ programmers, once you have figured out how to read the somewhat convoluted "standardese".
â Ben Steffan
Apr 29 at 8:56
add a comment |Â
up vote
6
down vote
accepted
You are missing the iterator_traits
types
If there were only one big tip I could give you, it would be to add the required type aliases for compatibility with the standard iterator interface. Citing from working draft N 4296 of the C++ standard (the last working draft before C++14), section 24.4.1 [iterator.traits]:
To implement algorithms only in terms of iterators, it is often
necessary to determine the value and difference types that correspond
to a particular iterator type. Accordingly, it is required that if
Iterator is the type of an iterator, the typesiterator_traits<Iterator>::difference_type
iterator_traits<Iterator>::value_type
iterator_traits<Iterator>::iterator_category
be defined as the
iteratorâÂÂs difference type, value type and iterator category,
respectively. In addition, the typesiterator_traits<Iterator>::reference
iterator_traits<Iterator>::pointer
shall be defined as the iteratorâÂÂs
reference and pointer types, that is, for an iterator objecta
, the
same type as the type of*a
anda->
, respectively. In the case of an
output iterator, the typesiterator_traits<Iterator>::difference_type
iterator_traits<Iterator>::value_type
iterator_traits<Iterator>::reference
iterator_traits<Iterator>::pointer
may be defined as
void
.
Thus, your implementation is currently not a valid iterator in the STL sense (which was also already noticed by nwp in the comments).
Your interface is a little confusing
The class that you wrote, indirection_iterator
, does not only serve as an iterator, but also as a kind of iterator factory. Code such as
indirection_iterator<decltype(alphabet.cbegin()),
decltype(char_indices.cbegin())> indirection_iter
alphabet.cbegin(),
alphabet.cend(),
char_indices.cbegin(),
char_indices.cend()
;
std::copy(indirection_iter.begin(),
indirection_iter.end(),
std::ostream_iterator<char>std::cout);
makes me feel really uneasy because it goes against how iterators are used by the STL. Calling begin
and end
on an iterator just feels wrong.
I suggest splitting up the functionality into two classes instead: One class which is used to make indirection iterators from a given set of value and index iterators (called indirection_iterator_factory
, for example) and an indirection_iterator
class which encapsulates only the iterator functionality.
De-cluttering
An added benefit of this method would be that you could reduce your actual iterator class by a few members. Another point which makes me uneasy is the fact that you have six different members in what is basically a simple iterator class, some of which are unnecessary as far as I can see. For example, you never make use of m_index_begin
, so that should be the first variable to go. The same is true for m_index_end
, as for m_element_end
.
Before we go on removing stuff, let's talk a bit about iterator design here. In particular, I want to talk about check_index
. So far, you are evaluating whether a particular index is in bounds for every access and, if it is not, throw an exception. That last part is going to make some people angry, because throwing exceptions is something a not negligible part of C++ developers disapprove of, and on some platforms, exceptions are not even usable at all. This does not mean that you should never rely on exceptions, but in my opinion, you should make everything relying on exceptions opt-in.
In your case, that basically means extracting the index check into a public interface method that the user can call if he wants, but doesn't have to. This is akin to how this issue is usually dealt with in the standard library: For example, std::vector
offers both an unchecked operator
as well as a checked (and exception-throwing) method at
.
Having said that, I do actually believe that iterators should be as minimalistic as possible and only offer what they were designed to do, which is, in most cases, iterating a certain range of objects. If I had to rewrite your indirection_iterator
, I would certainly leave out range checking as a whole, and require that all the indices in the underlying container be valid (again, it does seem to me that this largely reflects how the iterators in the STL were designed). This is a matter of opinion, however, so don't take this as a must-do.
Adding to the interface
The interface of indirection_iterator
is currently kind of meager. For example, operator->
is missing, which is a requirement for input iterators; an iterator category you could easily fulfill. The same is true for pre-increment, and having operator==
, although not strictly required, would probably be reasonable. Another thing which I would definitely add is a way to retrieve the current index, since this opens up a wide variety of possible use cases for your users, ranging from logging elements with their index to doing bounds checking manually.
Then there is a whole group of methods which are difficult to provide, but oftentimes very reasonable, e.g. operator--
. The difficulty here arises from the fact that what you have written is less of an actual iterator and more of an iterator adapter.
Optimizing functionality for different iterators could definitely be beneficial to usability as well as speed, but the big downside is that it will be a lot of work to implement. Ultimately, it depends on your use case, but if you really wanted to make this iterator part of a publicly available library, you should do that step. Otherwise, it is purely up to you whether you want to invest that much time and effort.
Little things and nitpicks
- The name of the type parameter to
check_index
,index_type
, is the same as an alias template you defined before. This could possibly lead to name shadowing or worse, so I would suggest you change either of those two names. - The template typename parameter
IndexIterator
does not convey what type of iterator you require. I would suggestIndexForwardIterator
. operator!=
is currently somewhat dysfunctional, the reason being that you only check for index equality, but not whether both of the iterators are iterators over the same container. Thus, if I had an iterator over container a with indices i and an iterator over container b with indices i, both iterators could still compare equal, which is likely not what you want.
IâÂÂm not sure about calling begin on iterator being wrong. May be you expected dereference first?
â Incomputable
Jan 18 at 6:46
1
@Incomputable No, this is more of a general idea of iterator design. An iterator is basically a generalization of a pointer, and is usually obtained from a container by callingbegin
andend
. Callingbegin
andend
on the iterator itself doesn't make much sense from a design point of view, because you are not iterating over the iterator but the underlying container(s) instead.
â Ben Steffan
Jan 18 at 14:36
Nice one! BTW, I am pretty sure that I can deal with basic C++ syntax, but would it be a good idea to read a standard draft as a "course book"?
â coderodde
Apr 29 at 8:52
1
@coderodde If you have the dexterity to read a whole draft, then there is probably a lot you can learn from that. I certainly don't, but in any case, I keep a standard draft around just to look up certain things from time to time. It's probably the most invaluable resource for C++ programmers, once you have figured out how to read the somewhat convoluted "standardese".
â Ben Steffan
Apr 29 at 8:56
add a comment |Â
up vote
6
down vote
accepted
up vote
6
down vote
accepted
You are missing the iterator_traits
types
If there were only one big tip I could give you, it would be to add the required type aliases for compatibility with the standard iterator interface. Citing from working draft N 4296 of the C++ standard (the last working draft before C++14), section 24.4.1 [iterator.traits]:
To implement algorithms only in terms of iterators, it is often
necessary to determine the value and difference types that correspond
to a particular iterator type. Accordingly, it is required that if
Iterator is the type of an iterator, the typesiterator_traits<Iterator>::difference_type
iterator_traits<Iterator>::value_type
iterator_traits<Iterator>::iterator_category
be defined as the
iteratorâÂÂs difference type, value type and iterator category,
respectively. In addition, the typesiterator_traits<Iterator>::reference
iterator_traits<Iterator>::pointer
shall be defined as the iteratorâÂÂs
reference and pointer types, that is, for an iterator objecta
, the
same type as the type of*a
anda->
, respectively. In the case of an
output iterator, the typesiterator_traits<Iterator>::difference_type
iterator_traits<Iterator>::value_type
iterator_traits<Iterator>::reference
iterator_traits<Iterator>::pointer
may be defined as
void
.
Thus, your implementation is currently not a valid iterator in the STL sense (which was also already noticed by nwp in the comments).
Your interface is a little confusing
The class that you wrote, indirection_iterator
, does not only serve as an iterator, but also as a kind of iterator factory. Code such as
indirection_iterator<decltype(alphabet.cbegin()),
decltype(char_indices.cbegin())> indirection_iter
alphabet.cbegin(),
alphabet.cend(),
char_indices.cbegin(),
char_indices.cend()
;
std::copy(indirection_iter.begin(),
indirection_iter.end(),
std::ostream_iterator<char>std::cout);
makes me feel really uneasy because it goes against how iterators are used by the STL. Calling begin
and end
on an iterator just feels wrong.
I suggest splitting up the functionality into two classes instead: One class which is used to make indirection iterators from a given set of value and index iterators (called indirection_iterator_factory
, for example) and an indirection_iterator
class which encapsulates only the iterator functionality.
De-cluttering
An added benefit of this method would be that you could reduce your actual iterator class by a few members. Another point which makes me uneasy is the fact that you have six different members in what is basically a simple iterator class, some of which are unnecessary as far as I can see. For example, you never make use of m_index_begin
, so that should be the first variable to go. The same is true for m_index_end
, as for m_element_end
.
Before we go on removing stuff, let's talk a bit about iterator design here. In particular, I want to talk about check_index
. So far, you are evaluating whether a particular index is in bounds for every access and, if it is not, throw an exception. That last part is going to make some people angry, because throwing exceptions is something a not negligible part of C++ developers disapprove of, and on some platforms, exceptions are not even usable at all. This does not mean that you should never rely on exceptions, but in my opinion, you should make everything relying on exceptions opt-in.
In your case, that basically means extracting the index check into a public interface method that the user can call if he wants, but doesn't have to. This is akin to how this issue is usually dealt with in the standard library: For example, std::vector
offers both an unchecked operator
as well as a checked (and exception-throwing) method at
.
Having said that, I do actually believe that iterators should be as minimalistic as possible and only offer what they were designed to do, which is, in most cases, iterating a certain range of objects. If I had to rewrite your indirection_iterator
, I would certainly leave out range checking as a whole, and require that all the indices in the underlying container be valid (again, it does seem to me that this largely reflects how the iterators in the STL were designed). This is a matter of opinion, however, so don't take this as a must-do.
Adding to the interface
The interface of indirection_iterator
is currently kind of meager. For example, operator->
is missing, which is a requirement for input iterators; an iterator category you could easily fulfill. The same is true for pre-increment, and having operator==
, although not strictly required, would probably be reasonable. Another thing which I would definitely add is a way to retrieve the current index, since this opens up a wide variety of possible use cases for your users, ranging from logging elements with their index to doing bounds checking manually.
Then there is a whole group of methods which are difficult to provide, but oftentimes very reasonable, e.g. operator--
. The difficulty here arises from the fact that what you have written is less of an actual iterator and more of an iterator adapter.
Optimizing functionality for different iterators could definitely be beneficial to usability as well as speed, but the big downside is that it will be a lot of work to implement. Ultimately, it depends on your use case, but if you really wanted to make this iterator part of a publicly available library, you should do that step. Otherwise, it is purely up to you whether you want to invest that much time and effort.
Little things and nitpicks
- The name of the type parameter to
check_index
,index_type
, is the same as an alias template you defined before. This could possibly lead to name shadowing or worse, so I would suggest you change either of those two names. - The template typename parameter
IndexIterator
does not convey what type of iterator you require. I would suggestIndexForwardIterator
. operator!=
is currently somewhat dysfunctional, the reason being that you only check for index equality, but not whether both of the iterators are iterators over the same container. Thus, if I had an iterator over container a with indices i and an iterator over container b with indices i, both iterators could still compare equal, which is likely not what you want.
You are missing the iterator_traits
types
If there were only one big tip I could give you, it would be to add the required type aliases for compatibility with the standard iterator interface. Citing from working draft N 4296 of the C++ standard (the last working draft before C++14), section 24.4.1 [iterator.traits]:
To implement algorithms only in terms of iterators, it is often
necessary to determine the value and difference types that correspond
to a particular iterator type. Accordingly, it is required that if
Iterator is the type of an iterator, the typesiterator_traits<Iterator>::difference_type
iterator_traits<Iterator>::value_type
iterator_traits<Iterator>::iterator_category
be defined as the
iteratorâÂÂs difference type, value type and iterator category,
respectively. In addition, the typesiterator_traits<Iterator>::reference
iterator_traits<Iterator>::pointer
shall be defined as the iteratorâÂÂs
reference and pointer types, that is, for an iterator objecta
, the
same type as the type of*a
anda->
, respectively. In the case of an
output iterator, the typesiterator_traits<Iterator>::difference_type
iterator_traits<Iterator>::value_type
iterator_traits<Iterator>::reference
iterator_traits<Iterator>::pointer
may be defined as
void
.
Thus, your implementation is currently not a valid iterator in the STL sense (which was also already noticed by nwp in the comments).
Your interface is a little confusing
The class that you wrote, indirection_iterator
, does not only serve as an iterator, but also as a kind of iterator factory. Code such as
indirection_iterator<decltype(alphabet.cbegin()),
decltype(char_indices.cbegin())> indirection_iter
alphabet.cbegin(),
alphabet.cend(),
char_indices.cbegin(),
char_indices.cend()
;
std::copy(indirection_iter.begin(),
indirection_iter.end(),
std::ostream_iterator<char>std::cout);
makes me feel really uneasy because it goes against how iterators are used by the STL. Calling begin
and end
on an iterator just feels wrong.
I suggest splitting up the functionality into two classes instead: One class which is used to make indirection iterators from a given set of value and index iterators (called indirection_iterator_factory
, for example) and an indirection_iterator
class which encapsulates only the iterator functionality.
De-cluttering
An added benefit of this method would be that you could reduce your actual iterator class by a few members. Another point which makes me uneasy is the fact that you have six different members in what is basically a simple iterator class, some of which are unnecessary as far as I can see. For example, you never make use of m_index_begin
, so that should be the first variable to go. The same is true for m_index_end
, as for m_element_end
.
Before we go on removing stuff, let's talk a bit about iterator design here. In particular, I want to talk about check_index
. So far, you are evaluating whether a particular index is in bounds for every access and, if it is not, throw an exception. That last part is going to make some people angry, because throwing exceptions is something a not negligible part of C++ developers disapprove of, and on some platforms, exceptions are not even usable at all. This does not mean that you should never rely on exceptions, but in my opinion, you should make everything relying on exceptions opt-in.
In your case, that basically means extracting the index check into a public interface method that the user can call if he wants, but doesn't have to. This is akin to how this issue is usually dealt with in the standard library: For example, std::vector
offers both an unchecked operator
as well as a checked (and exception-throwing) method at
.
Having said that, I do actually believe that iterators should be as minimalistic as possible and only offer what they were designed to do, which is, in most cases, iterating a certain range of objects. If I had to rewrite your indirection_iterator
, I would certainly leave out range checking as a whole, and require that all the indices in the underlying container be valid (again, it does seem to me that this largely reflects how the iterators in the STL were designed). This is a matter of opinion, however, so don't take this as a must-do.
Adding to the interface
The interface of indirection_iterator
is currently kind of meager. For example, operator->
is missing, which is a requirement for input iterators; an iterator category you could easily fulfill. The same is true for pre-increment, and having operator==
, although not strictly required, would probably be reasonable. Another thing which I would definitely add is a way to retrieve the current index, since this opens up a wide variety of possible use cases for your users, ranging from logging elements with their index to doing bounds checking manually.
Then there is a whole group of methods which are difficult to provide, but oftentimes very reasonable, e.g. operator--
. The difficulty here arises from the fact that what you have written is less of an actual iterator and more of an iterator adapter.
Optimizing functionality for different iterators could definitely be beneficial to usability as well as speed, but the big downside is that it will be a lot of work to implement. Ultimately, it depends on your use case, but if you really wanted to make this iterator part of a publicly available library, you should do that step. Otherwise, it is purely up to you whether you want to invest that much time and effort.
Little things and nitpicks
- The name of the type parameter to
check_index
,index_type
, is the same as an alias template you defined before. This could possibly lead to name shadowing or worse, so I would suggest you change either of those two names. - The template typename parameter
IndexIterator
does not convey what type of iterator you require. I would suggestIndexForwardIterator
. operator!=
is currently somewhat dysfunctional, the reason being that you only check for index equality, but not whether both of the iterators are iterators over the same container. Thus, if I had an iterator over container a with indices i and an iterator over container b with indices i, both iterators could still compare equal, which is likely not what you want.
answered Jan 17 at 21:42
Ben Steffan
4,85011234
4,85011234
IâÂÂm not sure about calling begin on iterator being wrong. May be you expected dereference first?
â Incomputable
Jan 18 at 6:46
1
@Incomputable No, this is more of a general idea of iterator design. An iterator is basically a generalization of a pointer, and is usually obtained from a container by callingbegin
andend
. Callingbegin
andend
on the iterator itself doesn't make much sense from a design point of view, because you are not iterating over the iterator but the underlying container(s) instead.
â Ben Steffan
Jan 18 at 14:36
Nice one! BTW, I am pretty sure that I can deal with basic C++ syntax, but would it be a good idea to read a standard draft as a "course book"?
â coderodde
Apr 29 at 8:52
1
@coderodde If you have the dexterity to read a whole draft, then there is probably a lot you can learn from that. I certainly don't, but in any case, I keep a standard draft around just to look up certain things from time to time. It's probably the most invaluable resource for C++ programmers, once you have figured out how to read the somewhat convoluted "standardese".
â Ben Steffan
Apr 29 at 8:56
add a comment |Â
IâÂÂm not sure about calling begin on iterator being wrong. May be you expected dereference first?
â Incomputable
Jan 18 at 6:46
1
@Incomputable No, this is more of a general idea of iterator design. An iterator is basically a generalization of a pointer, and is usually obtained from a container by callingbegin
andend
. Callingbegin
andend
on the iterator itself doesn't make much sense from a design point of view, because you are not iterating over the iterator but the underlying container(s) instead.
â Ben Steffan
Jan 18 at 14:36
Nice one! BTW, I am pretty sure that I can deal with basic C++ syntax, but would it be a good idea to read a standard draft as a "course book"?
â coderodde
Apr 29 at 8:52
1
@coderodde If you have the dexterity to read a whole draft, then there is probably a lot you can learn from that. I certainly don't, but in any case, I keep a standard draft around just to look up certain things from time to time. It's probably the most invaluable resource for C++ programmers, once you have figured out how to read the somewhat convoluted "standardese".
â Ben Steffan
Apr 29 at 8:56
IâÂÂm not sure about calling begin on iterator being wrong. May be you expected dereference first?
â Incomputable
Jan 18 at 6:46
IâÂÂm not sure about calling begin on iterator being wrong. May be you expected dereference first?
â Incomputable
Jan 18 at 6:46
1
1
@Incomputable No, this is more of a general idea of iterator design. An iterator is basically a generalization of a pointer, and is usually obtained from a container by calling
begin
and end
. Calling begin
and end
on the iterator itself doesn't make much sense from a design point of view, because you are not iterating over the iterator but the underlying container(s) instead.â Ben Steffan
Jan 18 at 14:36
@Incomputable No, this is more of a general idea of iterator design. An iterator is basically a generalization of a pointer, and is usually obtained from a container by calling
begin
and end
. Calling begin
and end
on the iterator itself doesn't make much sense from a design point of view, because you are not iterating over the iterator but the underlying container(s) instead.â Ben Steffan
Jan 18 at 14:36
Nice one! BTW, I am pretty sure that I can deal with basic C++ syntax, but would it be a good idea to read a standard draft as a "course book"?
â coderodde
Apr 29 at 8:52
Nice one! BTW, I am pretty sure that I can deal with basic C++ syntax, but would it be a good idea to read a standard draft as a "course book"?
â coderodde
Apr 29 at 8:52
1
1
@coderodde If you have the dexterity to read a whole draft, then there is probably a lot you can learn from that. I certainly don't, but in any case, I keep a standard draft around just to look up certain things from time to time. It's probably the most invaluable resource for C++ programmers, once you have figured out how to read the somewhat convoluted "standardese".
â Ben Steffan
Apr 29 at 8:56
@coderodde If you have the dexterity to read a whole draft, then there is probably a lot you can learn from that. I certainly don't, but in any case, I keep a standard draft around just to look up certain things from time to time. It's probably the most invaluable resource for C++ programmers, once you have figured out how to read the somewhat convoluted "standardese".
â Ben Steffan
Apr 29 at 8:56
add a comment |Â
up vote
3
down vote
A portability problem
I know this is just the test code, rather than the real subject of the review, but it's worth mentioning.
for (char ch = 'a'; ch <= 'z'; ch++)
alphabet.push_back(ch);
This doesn't necessarily do what you think it does; it depends on the target's character coding. In EBCDIC locales, this will insert 41 values, which will throw your indexing off.
It's safer and clearer to initialize from a string literal:
static std::vector<char> get_alphabet()
static char const s = ""
"abcdefghijklmnopqrstuvwxyz"
"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
", !n";
return std::begin(s), std::end(s);
Iterator interface
GCC warns (with -Weffc++
) that operator++()
should conventionally return a reference to *this
, not void
. Many of the methods could/should be declared const
, too (at least begin
, end
, operator*
, operator!=
and check_index
, per my testing).
User Incomputable recommended that I point you at operator overloading (on cppreference.com) as a useful list of the usual function signatures.
BTW, if anyone is wondering - that initial""
is there just to help Emacs line up the rows ofs
.
â Toby Speight
Jan 18 at 14:15
add a comment |Â
up vote
3
down vote
A portability problem
I know this is just the test code, rather than the real subject of the review, but it's worth mentioning.
for (char ch = 'a'; ch <= 'z'; ch++)
alphabet.push_back(ch);
This doesn't necessarily do what you think it does; it depends on the target's character coding. In EBCDIC locales, this will insert 41 values, which will throw your indexing off.
It's safer and clearer to initialize from a string literal:
static std::vector<char> get_alphabet()
static char const s = ""
"abcdefghijklmnopqrstuvwxyz"
"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
", !n";
return std::begin(s), std::end(s);
Iterator interface
GCC warns (with -Weffc++
) that operator++()
should conventionally return a reference to *this
, not void
. Many of the methods could/should be declared const
, too (at least begin
, end
, operator*
, operator!=
and check_index
, per my testing).
User Incomputable recommended that I point you at operator overloading (on cppreference.com) as a useful list of the usual function signatures.
BTW, if anyone is wondering - that initial""
is there just to help Emacs line up the rows ofs
.
â Toby Speight
Jan 18 at 14:15
add a comment |Â
up vote
3
down vote
up vote
3
down vote
A portability problem
I know this is just the test code, rather than the real subject of the review, but it's worth mentioning.
for (char ch = 'a'; ch <= 'z'; ch++)
alphabet.push_back(ch);
This doesn't necessarily do what you think it does; it depends on the target's character coding. In EBCDIC locales, this will insert 41 values, which will throw your indexing off.
It's safer and clearer to initialize from a string literal:
static std::vector<char> get_alphabet()
static char const s = ""
"abcdefghijklmnopqrstuvwxyz"
"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
", !n";
return std::begin(s), std::end(s);
Iterator interface
GCC warns (with -Weffc++
) that operator++()
should conventionally return a reference to *this
, not void
. Many of the methods could/should be declared const
, too (at least begin
, end
, operator*
, operator!=
and check_index
, per my testing).
User Incomputable recommended that I point you at operator overloading (on cppreference.com) as a useful list of the usual function signatures.
A portability problem
I know this is just the test code, rather than the real subject of the review, but it's worth mentioning.
for (char ch = 'a'; ch <= 'z'; ch++)
alphabet.push_back(ch);
This doesn't necessarily do what you think it does; it depends on the target's character coding. In EBCDIC locales, this will insert 41 values, which will throw your indexing off.
It's safer and clearer to initialize from a string literal:
static std::vector<char> get_alphabet()
static char const s = ""
"abcdefghijklmnopqrstuvwxyz"
"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
", !n";
return std::begin(s), std::end(s);
Iterator interface
GCC warns (with -Weffc++
) that operator++()
should conventionally return a reference to *this
, not void
. Many of the methods could/should be declared const
, too (at least begin
, end
, operator*
, operator!=
and check_index
, per my testing).
User Incomputable recommended that I point you at operator overloading (on cppreference.com) as a useful list of the usual function signatures.
edited Jan 18 at 14:27
answered Jan 18 at 12:40
Toby Speight
17.8k13491
17.8k13491
BTW, if anyone is wondering - that initial""
is there just to help Emacs line up the rows ofs
.
â Toby Speight
Jan 18 at 14:15
add a comment |Â
BTW, if anyone is wondering - that initial""
is there just to help Emacs line up the rows ofs
.
â Toby Speight
Jan 18 at 14:15
BTW, if anyone is wondering - that initial
""
is there just to help Emacs line up the rows of s
.â Toby Speight
Jan 18 at 14:15
BTW, if anyone is wondering - that initial
""
is there just to help Emacs line up the rows of s
.â Toby Speight
Jan 18 at 14:15
add a comment |Â
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f185343%2fa-c-custom-iterator-for-indirection-iteration%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
1
Does not compile. I think it's because the iterator traits are supposed to be discoverable by (standard) algorithms, so you can't make them
private
.â nwp
Jan 17 at 20:23
@nwp Did on Xcode yet I had to set C++14 in the settings. I will take a look at the issue but not today.
â coderodde
Jan 17 at 20:26