member iterator - request for comment
in [C++]
|
From: Mikosz on 20 Jun 2010 22:06 Hi, Recently I've met the need to perform operations like this quite often: struct A { int val; } .... std::vector<A> as; .... std::set<int> vals; std::vector<A>::const_iterator it, end = as.end(); for (it = as.begin(); it != end; ++it) { vals.insert(it->val); } that is, to perform some action on one of the members of each of the collection's elements. I couldn't find any solutions within the STL, so I've created my own MemberIterator class. The code is: template<class Iterator, class PointerToMember, class Member> class MemberIterator : public Iterator { public: MemberIterator(Iterator it, PointerToMember ptr) : Iterator(it), ptr_(ptr) { } Member& operator*() { return Iterator::operator*().*ptr_; } private: PointerToMember ptr_; }; template<class Member, class Iterator, class PointerToMember> MemberIterator<Iterator, PointerToMember, Member> makeMemberIterator( Iterator it, PointerToMember ptr) { return MemberIterator<Iterator, PointerToMember, Member>(it, ptr); } Given this solution, I can write: std::copy(makeMemberIterator<int>(as.begin(), &A::val), makeMemberIterator<int>(as.end(), &A::val), std::back_inserter(vals)); and it works just fine. I would like to know your opinion on 1. whether this class is implemented correctly or how I could make it better 2. whether I should attempt to write stuff like this or use some STL provided solution that I'm not aware of Thanks, Mikolaj -- [ See http://www.gotw.ca/resources/clcm.htm for info about ] [ comp.lang.c++.moderated. First time posters: Do this! ]
From: Paul Bibbings on 21 Jun 2010 08:35 Mikosz <mikoszrrr(a)gmail.com> writes: > Hi, > > Recently I've met the need to perform operations like this quite > often: > > struct A { > int val; > } > > ... > std::vector<A> as; > ... > std::set<int> vals; > std::vector<A>::const_iterator it, end = as.end(); > for (it = as.begin(); it != end; ++it) { > vals.insert(it->val); > } > > that is, to perform some action on one of the members of each of the > collection's elements. I couldn't find any solutions within the STL, #include <vector> #include <set> #include <algorithm> struct A { A(int v) : val(v) { } // added for convenience here int val; }; struct val_functor { int operator()(A a) { return a.val; } }; int main() { std::vector<A> as; std::set<int> vals; for (int i = 0; i < 10; i++) as.push_back(i); std::transform(as.begin(), as.end(), std::inserter(vals, vals.begin()), val_functor()); } > so I've created my own MemberIterator class. The code is: > > template<class Iterator, class PointerToMember, class Member> > class MemberIterator : public Iterator { > public: > > MemberIterator(Iterator it, PointerToMember ptr) : Iterator(it), > ptr_(ptr) { > } > > Member& operator*() { > return Iterator::operator*().*ptr_; > } > > private: > > PointerToMember ptr_; > > }; > > template<class Member, class Iterator, class PointerToMember> > MemberIterator<Iterator, PointerToMember, Member> makeMemberIterator( > Iterator it, PointerToMember ptr) { > return MemberIterator<Iterator, PointerToMember, Member>(it, ptr); > } > > Given this solution, I can write: > > std::copy(makeMemberIterator<int>(as.begin(), &A::val), > makeMemberIterator<int>(as.end(), &A::val), std::back_inserter(vals)); > > and it works just fine. I wouldn't have thought that the above would work at all. Given that your vals is of type std::set<int>, I would have expected the above line of code to fail on std::set not defining push_back. > I would like to know your opinion on > 1. whether this class is implemented correctly or how I could make it > better > 2. whether I should attempt to write stuff like this or use some STL > provided solution that I'm not aware of Prefer the second option in 2. which, effectively, makes 1. moot. Regards Paul Bibbings -- [ See http://www.gotw.ca/resources/clcm.htm for info about ] [ comp.lang.c++.moderated. First time posters: Do this! ]
From: Nick Hounsome on 22 Jun 2010 08:28 On 21 June, 14:06, Mikosz <mikosz...(a)gmail.com> wrote: > Hi, > > Recently I've met the need to perform operations like this quite > often: > > struct A { > int val; > > } > > ... > std::vector<A> as; > ... > std::set<int> vals; > std::vector<A>::const_iterator it, end = as.end(); > for (it = as.begin(); it != end; ++it) { > vals.insert(it->val); > > } > > that is, to perform some action on one of the members of each of the > collection's elements. I couldn't find any solutions within the STL, > so I've created my own MemberIterator class. The code is: My first thought was to make a decorator collection that presents a (obviouly const) collection as a collection of some member. It would work for your example but would otherwise be less flexible. My second thought was to wonder whether you could just deduce the type from the member pointer. My third thought - and it should probably have been my first - is that your result is far less readable than the original. Maybe it's me but I find explicit loops to be more comprehensible in 90% of cases. -- [ See http://www.gotw.ca/resources/clcm.htm for info about ] [ comp.lang.c++.moderated. First time posters: Do this! ]
From: Mikosz on 22 Jun 2010 08:45 Paul, thanks for the quick reply. On 22 Cze, 01:35, Paul Bibbings <paul.bibbi...(a)gmail.com> wrote: > Mikosz <mikosz...(a)gmail.com> writes: > > [...] to perform some action on one of the members of each of the > > collection's elements. I couldn't find any solutions within the STL, > > #include <vector> > #include <set> > #include <algorithm> > > struct A { > A(int v) : val(v) { } // added for convenience here > int val; > }; > > struct val_functor { > int operator()(A a) { return a.val; } > }; > > int main() > { > std::vector<A> as; > std::set<int> vals; > for (int i = 0; i < 10; i++) as.push_back(i); > > std::transform(as.begin(), > as.end(), > std::inserter(vals, vals.begin()), > val_functor()); > } > Good point, I actually forgot about 'transform'. However, a solution that would allow me not to implement an extracting functor would be much neater. Again - I don't know of any existing solutions, so I crafted my own. What do you think of this: template<class PointerToMember, class Member> class Extractor { public: Extractor(PointerToMember ptr) : ptr_(ptr) { } template<class T> Member& operator()(T& object) { return object.*ptr_; } template<class T> const Member& operator()(const T& object) const { return object.*ptr_; } private: PointerToMember ptr_; }; template <class Member, class PointerToMember> Extractor<PointerToMember, Member> makeExtractor(PointerToMember ptr) { return Extractor<PointerToMember, Member>(ptr); } struct A { A(int v) : val(v) { } int val; }; int main() { std::vector<A> as; std::set<int> vals; for (int i = 0; i < 10; i++) { as.push_back(i); } std::transform(as.begin(), as.end(), std::inserter(vals, vals.begin()), makeExtractor<int>(&A::val)); std::copy(vals.begin(), vals.end(), std::ostream_iterator<int>(std::cout, ", ")); } > > > std::copy(makeMemberIterator<int>(as.begin(), &A::val), > > makeMemberIterator<int>(as.end(), &A::val), std::back_inserter(vals)); > > > and it works just fine. > > I wouldn't have thought that the above would work at all. Given that > your vals is of type std::set<int>, I would have expected the above line > of code to fail on std::set not defining push_back. > Sure, that was a typing error. Should be std::inserter instead of std::back_inserter in there. -- [ See http://www.gotw.ca/resources/clcm.htm for info about ] [ comp.lang.c++.moderated. First time posters: Do this! ]
From: Paul Bibbings on 22 Jun 2010 15:27
Mikosz <mikoszrrr(a)gmail.com> writes: > Paul, thanks for the quick reply. > > On 22 Cze, 01:35, Paul Bibbings <paul.bibbi...(a)gmail.com> wrote: >> Mikosz <mikosz...(a)gmail.com> writes: >> > [...] to perform some action on one of the members of each of the >> > collection's elements. I couldn't find any solutions within the STL, >> >> #include <vector> >> #include <set> >> #include <algorithm> >> >> struct A { >> A(int v) : val(v) { } // added for convenience here >> int val; >> }; >> >> struct val_functor { >> int operator()(A a) { return a.val; } >> }; >> >> int main() >> { >> std::vector<A> as; >> std::set<int> vals; >> for (int i = 0; i < 10; i++) as.push_back(i); >> >> std::transform(as.begin(), >> as.end(), >> std::inserter(vals, vals.begin()), >> val_functor()); >> } >> > > Good point, I actually forgot about 'transform'. However, a solution > that would allow me not to implement an extracting functor would be > much neater. Again - I don't know of any existing solutions, so I > crafted my own. What do you think of this: From the `solution' that you give below, what I understand you to be requiring here is, not that you don't want to implement an extracting functor as such (since, essentially, that is what you do), but that you don't want to have to implement *separate* functors for different classes and for different members of those classes; essentially, what you are looking for is a /generic/ solution. Would that be right? > template<class PointerToMember, class Member> > class Extractor { > public: > > Extractor(PointerToMember ptr) : > ptr_(ptr) { > } > > template<class T> > Member& operator()(T& object) { > return object.*ptr_; > } > > template<class T> > const Member& operator()(const T& object) const { > return object.*ptr_; > } > > private: > > PointerToMember ptr_; > > }; > > template <class Member, class PointerToMember> > Extractor<PointerToMember, Member> makeExtractor(PointerToMember ptr) > { > return Extractor<PointerToMember, Member>(ptr); > } > > struct A { > A(int v) : > val(v) { > } > int val; > }; > > int main() { > std::vector<A> as; > std::set<int> vals; > for (int i = 0; i < 10; i++) { > as.push_back(i); > } > > std::transform(as.begin(), as.end(), std::inserter(vals, > vals.begin()), makeExtractor<int>(&A::val)); > std::copy(vals.begin(), vals.end(), > std::ostream_iterator<int>(std::cout, ", ")); > } Given the aim as I have understood it, I think that you can do just a little better and get template argument deduction to do more for you. Also, you don't need both of your overloads for the function call operator. Since neither modify the object on which the operator is called, the const version is sufficient. Putting all this together, I might suggest something like the following, which is, essentially, a refactoring of your idea applying a `better' parameterization: #include <vector> #include <set> #include <algorithm> #include <iostream> #include <iterator> template<class T, class M> class Extractor { public: Extractor(M (T::*ptr)) : ptr_(ptr) { } M operator()(const T& t) { return t.*ptr_; } private: M (T::*ptr_); }; template<class T, class M> // *both* T and M can be deduced here Extractor<T, M> makeExtractor(M (T::*ptr)) { return Extractor<T, M>(ptr); } struct A { A(int i) : val(i) { } int val; }; int main() { std::vector<A> as; std::set<int> vals; for (int i = 0; i < 10; i++) { as.push_back(i); } std::transform(as.begin(), as.end(), std::inserter(vals, vals.begin()), makeExtractor(&A::val)); std::copy(vals.begin(), vals.end(), std::ostream_iterator<int>(std::cout, ", ")); } Here, the main difference is that *both* template arguments to makeExtractor can be deduced from the single function call argument whereas, in your implementation, you had to provide: makeExtractor<int>(&A::val); Regards Paul Bibbings -- [ See http://www.gotw.ca/resources/clcm.htm for info about ] [ comp.lang.c++.moderated. First time posters: Do this! ] |