Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

My goal was to get the following code to work:

#include<iostream>
#include<vector>
#include<list>
#include<algorithm>
#include<array>
#include"functional.hpp"

int main( )
{
    std::vector<double> a{1.0, 2.0, 3.0, 4.0};
    std::list<char> b;
    b.push_back('a');
    b.push_back('b');
    b.push_back('c');
    b.push_back('d');
    std::array<int,5> c{5,4,3,2,1};

    auto d = zip(a, b, c);

    for (auto i : zip(a, b, c) )
    {
        std::cout << std::get<0>(i) << ", " << std::get<1>(i) << ", " << std::get<2>(i) << std::endl;
    }

    for (auto i : d)
    {
        std::cout << std::get<0>(i) << ", " << std::get<1>(i) << ", " << std::get<2>(i) << std::endl;
        std::get<0>(i) = 5.0;
        //std::cout << i1 << ", " << i2 << ", " << i3 << std::endl;
    }
    for (auto i : d)
    {
        std::cout << std::get<0>(i) << ", " << std::get<1>(i) << ", " << std::get<2>(i) << std::endl;
        //std::cout << i1 << ", " << i2 << ", " << i3 << std::endl;
    }
}

With the output:

1, a, 5
2, b, 4
3, c, 3
4, d, 2
5, a, 5
5, b, 4
5, c, 3
5, d, 2

The source for "functional.hpp" is:

#pragma once
#include<tuple>
#include<iterator>
#include<utility>

/***************************
// helper for tuple_subset and tuple_tail (from http://stackoverflow.com/questions/8569567/get-part-of-stdtuple)
***************************/
template <size_t... n>
struct ct_integers_list {
    template <size_t m>
    struct push_back
    {
        typedef ct_integers_list<n..., m> type;
    };
};

template <size_t max>
struct ct_iota_1
{
    typedef typename ct_iota_1<max-1>::type::template push_back<max>::type type;
};

template <>
struct ct_iota_1<0>
{
    typedef ct_integers_list<> type;
};

/***************************
// return a subset of a tuple
***************************/
template <size_t... indices, typename Tuple>
auto tuple_subset(const Tuple& tpl, ct_integers_list<indices...>)
    -> decltype(std::make_tuple(std::get<indices>(tpl)...))
{
    return std::make_tuple(std::get<indices>(tpl)...);
    // this means:
    //   make_tuple(get<indices[0]>(tpl), get<indices[1]>(tpl), ...)
}

/***************************
// return the tail of a tuple
***************************/
template <typename Head, typename... Tail>
inline std::tuple<Tail...> tuple_tail(const std::tuple<Head, Tail...>& tpl)
{
    return tuple_subset(tpl, typename ct_iota_1<sizeof...(Tail)>::type());
    // this means:
    //   tuple_subset<1, 2, 3, ..., sizeof...(Tail)-1>(tpl, ..)
}

/***************************
// increment every element in a tuple (that is referenced)
***************************/
template<std::size_t I = 0, typename... Tp>
inline typename std::enable_if<I == sizeof...(Tp), void>::type
increment(std::tuple<Tp...>& t)
{ }

template<std::size_t I = 0, typename... Tp>
inline typename std::enable_if<(I < sizeof...(Tp)), void>::type
increment(std::tuple<Tp...>& t)
{
    std::get<I>(t)++ ;
    increment<I + 1, Tp...>(t);
}

/**************************** 
// check equality of a tuple
****************************/
template<typename T1>
inline bool not_equal_tuples( const std::tuple<T1>& t1,  const std::tuple<T1>& t2 )
{
    return (std::get<0>(t1) != std::get<0>(t2));
}

template<typename T1, typename... Ts>
inline bool not_equal_tuples( const std::tuple<T1, Ts...>& t1,  const std::tuple<T1, Ts...>& t2 )
{
    return (std::get<0>(t1) != std::get<0>(t2)) && not_equal_tuples( tuple_tail(t1), tuple_tail(t2) );
}

/**************************** 
// dereference a subset of elements of a tuple (dereferencing the iterators)
****************************/
template <size_t... indices, typename Tuple>
auto dereference_subset(const Tuple& tpl, ct_integers_list<indices...>)
    -> decltype(std::tie(*std::get<indices-1>(tpl)...))
{
    return std::tie(*std::get<indices-1>(tpl)...);
}

/**************************** 
// dereference every element of a tuple (applying operator* to each element, and returning the tuple)
****************************/
template<typename... Ts>
inline auto
  dereference_tuple(std::tuple<Ts...>& t1) -> decltype( dereference_subset( std::tuple<Ts...>(), typename ct_iota_1<sizeof...(Ts)>::type()))
  {
    return dereference_subset( t1, typename ct_iota_1<sizeof...(Ts)>::type());
  }


template< typename T1, typename... Ts >
class zipper
{
    public:

    class iterator : std::iterator<std::forward_iterator_tag, std::tuple<typename T1::value_type, typename Ts::value_type...> >
    {
        protected:
            std::tuple<typename T1::iterator, typename Ts::iterator...> current;
        public:

        explicit iterator(  typename T1::iterator s1, typename Ts::iterator... s2 ) : 
            current(s1, s2...) {};

        iterator( const iterator& rhs ) :  current(rhs.current) {};

        iterator& operator++() {
            increment(current);
            return *this;
        }

        iterator operator++(int) {
            auto a = *this;
            increment(current);
            return a;
        }

        bool operator!=( const iterator& rhs ) {
            return not_equal_tuples(current, rhs.current);
        }

        typename iterator::value_type operator*() {
            return dereference_tuple(current);
        }
    };


    explicit zipper( T1& a, Ts&... b):
                        begin_( a.begin(), (b.begin())...), 
                        end_( a.end(), (b.end())...) {};

    zipper(const zipper<T1, Ts...>& a) :
                        begin_(  a.begin_ ), 
                        end_( a.end_ ) {};

    template<typename U1, typename... Us>
    zipper<U1, Us...>& operator=( zipper<U1, Us...>& rhs) {
        begin_ = rhs.begin_;
        end_ = rhs.end_;
        return *this;
    }

    zipper<T1, Ts...>::iterator& begin() {
        return begin_;
    }

    zipper<T1, Ts...>::iterator& end() {
        return end_;
    }

    zipper<T1, Ts...>::iterator begin_;
    zipper<T1, Ts...>::iterator end_;
};



//from cppreference.com: 
template <class T>
  struct special_decay
  {
     using type = typename std::decay<T>::type;
  };

//allows the use of references:
template <class T>
 struct special_decay<std::reference_wrapper<T>>
 {
   using type = T&;
 };

template <class T>
 using special_decay_t = typename special_decay<T>::type;

//allows template type deduction for zipper:
template <class... Types>
 zipper<special_decay_t<Types>...> zip(Types&&... args)
 {
   return zipper<special_decay_t<Types>...>(std::forward<Types>(args)...);
 }

I'm asking for a few things:

  • References and performance: Theoretically, the only things that (I believe) should be copied are iterators, but I'm not sure how to confirm this. I'm hoping that this has very little overhead (A couple of pointer dereferences maybe?), but I'm not sure how to check something like this.

  • Correctness: Are there any hidden bugs that aren't showing up in my use case? Technically, the code isn't really working as "expected" (it should spit out copies of the values for "auto i", and only allow modification of the original containers values with "auto& i", and there should be a version that allows you to look, but not touch: "const auto& i"), but I'm not sure how to fix that. I expect I need to create a const version for the const auto& i mode, but I'm not sure how to make a copy for the auto i version.

  • Code Cleanliness, Best practices: My code is pretty much never read by another human being, so any recommendations of best practices or commenting would be appreciated. I'm also not sure what to do about the move constructors: should I be deleting them, or ignoring them?

share|improve this question
add comment

2 Answers

up vote 6 down vote accepted

There is nothing much to say. Your code is already quite good and quite clear. Here are a few tidbits though:

typedef

If you are willing to write modern code, you should consider dropping typedef and using using everywhere instead. It helps to be consistent between regular alias and alias template. Moreover, the = symbol help to visually split the new name and the type it refers to. And the syntax is also consistent towards the way you can declare variables:

auto i = 1;
using some_type = int;

Perfect forwarding

It's clear that you already used it. But there are some other places where it would make sense to use it:

template <size_t... indices, typename Tuple>
auto tuple_subset(Tuple&& tpl, ct_integers_list<indices...>)
    -> decltype(std::make_tuple(std::get<indices>(std::forward<Tuple>(tpl))...))
{
    return std::make_tuple(std::get<indices>(std::forward<Tuple>(tpl))...);
    // this means:
    //   make_tuple(get<indices[0]>(tpl), get<indices[1]>(tpl), ...)
}

std::enable_if

While using std::enable_if in the return type of the functions, I find that it tends to make it unreadable. Therefore, you may probably want to move it to the template parameters list instead. Consider your code:

template<std::size_t I = 0, typename... Tp>
inline typename std::enable_if<(I < sizeof...(Tp)), void>::type
increment(std::tuple<Tp...>& t)
{
    std::get<I>(t)++ ;
    increment<I + 1, Tp...>(t);
}

And compare it to this one:

template<std::size_t I = 0, typename... Tp,
        typename = typename std::enable_if<I < sizeof...(Tp), void>::type>
inline void increment(std::tuple<Tp...>& t)
{
    std::get<I>(t)++ ;
    increment<I + 1, Tp...>(t);
}

Pre-increment vs. post-increment

Depending on the type, ++var may be faster than var++. It does not change anything for int but if your container contains large type, remember that the ++ in var++ is generally defined as:

auto operator++(int)
    -> T&
{
    auto res = var;
    ++var;
    return res;
}

As you can see, yet another copy of the incremented variable is made and ++var is called. Therefore, you may want to use ++var instead of var++ in a generic context.

Miscellaneous tidbits

template<typename U1, typename... Us>
zipper<U1, Us...>& operator=(zipper<U1, Us...>& rhs) { ... }

You may want to pass a const zipper<U1, Us...>& instead of a zipper<U1, Us...>&.

zipper<T1, Ts...>::iterator& begin() {
    return begin_;
}

zipper<T1, Ts...>::iterator& end() {
    return end_;
}

You may also want to provide the functions begin() const, end() const, cbegin() const and cend() const if you want this set of functions to be complete and consistent towards the STL. Also, some operator== would be great for zipper::iterator.

Also, I like the new function syntax which IMHO helps to separate the function return type and its name, which is especially useful when the said return type is long. However, I read that some people don't like it, so it's up to your own preferences.

Conclusion

Generally speaking, your code was good and it works, which is even better. I've provided a few tips, but there are probably many other things that you could do to improve it. It will always be up to you when it concerns the readability though, preferences matter in that domain :)

share|improve this answer
    
Thanks for the feedback. I really appreciate it. –  Andrew Spott Nov 16 '13 at 2:47
add comment

Some of your metaprogramming machinery is a bit over-complicated for C++11.

Compare:

template <size_t... n>
struct ct_integers_list {
    template <size_t m>
    struct push_back
    {
        typedef ct_integers_list<n..., m> type;
    };
};

template <size_t max>
struct ct_iota_1
{
    typedef typename ct_iota_1<max-1>::type::template push_back<max>::type type;
};

template <>
struct ct_iota_1<0>
{
    typedef ct_integers_list<> type;
};
template<std::size_t I = 0, typename... Tp>
inline typename std::enable_if<I == sizeof...(Tp), void>::type
increment(std::tuple<Tp...>& t)
{ }

template<std::size_t I = 0, typename... Tp>
inline typename std::enable_if<(I < sizeof...(Tp)), void>::type
increment(std::tuple<Tp...>& t)
{
    std::get<I>(t)++ ;
    increment<I + 1, Tp...>(t);
}

Versus:

template<size_t... n> struct ct_integers_list {};

template<size_t... acc> struct ct_iota_1;
template<size_t max, size_t... acc> struct ct_iota_1 : ct_iota_1<max-1, max-1, acc...> {};
template<size_t... acc> struct ct_iota_1<0, acc...> : ct_integers_list<acc...> {};

template<size_t... Indices, typename Tuple>
inline void increment_helper(Tuple& t, ct_integers_list<Indices...>)
{
    std::initializer_list<int>{
        [&]{ ++std::get<Indices>(t); return 0; }()...
    };
}

template<typename... Tp>
inline void increment(std::tuple<Tp...>& t)
{
    increment_helper(t, ct_iota_1<sizeof...(Tp)>());
}

The idea is to let parameter-pack expansion do for you all the things that in C++03 you had to do via foo<T>::type typedefs and std::enable_if and so on.

Basically, you can replace a lot of recursion with iteration (as in my increment_helper); and for the stuff that has to remain recursion, you can make it look a bit neater and avoid the proliferation of entities (à la Occam's Razor). If you don't need all those intermediate ct_iota_1<...>::type entities, get rid of them!

However, if you want a production-quality ct_integers_list, you should be using C++14's predefined std::integer_sequence, or at least an efficient implementation such as this one by Xeo. Compilers often limit template recursion to something like 256 levels; both your version and mine will quickly run into this limit, whereas Xeo's will work fine, because its recursion is O(log max) rather than O(max).

share|improve this answer
    
Why did you edit away the MathJax notation? :o –  Morwenn Apr 29 at 8:05
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.