7

I have a class X, which I provide a snippet of here:

class X {
  public:
    template <typename Iter>
    X(Iter begin, Iter end) : mVec(begin, end) {}

  private:
    vector<Y> const mVec;
};

I now want to add a new concatenating constructor to this class, something like:

template <typename Iter1, typename Iter2>
X(Iter1 begin1, Iter1 end1, Iter2 begin2, Iter2 end2) : mVec(???) { ??? }

Such a constructor would catenate the two ranges [begin1, end1) and [begin2, end2) into mVec. The challenges are

1) I would like to preserve the const on mVec, so that it is considered constant throughout the other methods of X.

2) I would like to avoid unnecessary copies if at all possible. That is, one solution is to have a static method that constructs a non-const temporary to range 1, inserts range 2 and returns it, and then define the concatenating constructor to

template <typename Iter1, typename Iter2>
X(Iter1 begin1, Iter1 end1, Iter2 begin2, Iter2 end2) 
  : mVec(concatenate(begin1, end1, begin2, end2)) { }

but that copies all the values at least one extra time, I believe.

SCFrench
  • 8,244
  • 2
  • 31
  • 61

5 Answers5

9

Nice problem. I would try to implement a particular iterator wrapper type that turns the two ranges into a single range. Something in the lines of:

// compacted syntax for brevity...
template <typename T1, typename T2>
struct concat_iterator
{
public:
   typedef std::forward_iterator_tag iterator_category;
   typedef typename iterator_traits<T1>::value_type value_type;
   typedef *value_type pointer; 
   typedef &value_type reference;

   concat_iterator( T1 b1, T1 e1, T2 b2, T2 e2 ) 
      : seq1( b1 ), seq1end( e1 ), seq2( b2 ), seq2end( e2 );
   iterator& operator++() {
      if ( seq1 != seq1end ) ++seq1;
      else ++seq2;
      return this;
   }
   reference operator*() {
      if ( seq1 != seq1end ) return *seq1;
      else return *seq2;
   }
   pointer operator->() {
      if ( seq1 != seq1end ) return &(*seq1);
      else return &(*seq2);
   }
   bool operator==( concat_iterator const & rhs ) {
      return seq1==rhs.seq1 && seq1end==rhs.seq2 
          && seq2==rhs.seq2 && seq2end==rhs.seq2end;
   }
   bool operator!=( contact_iterator const & rhs ) {
      return !(*this == rhs);
   }
private:
   T1 seq1;
   T1 seq1end;
   T2 seq2;
   T2 seq2end;
};

template <typename T1, typename T2>
concat_iterator<T1,T2> concat_begin( T1 b1, T1 e1, T2 b2, T2 e2 )
{
   return concat_iterator<T1,T2>(b1,e1,b2,e2);
}
template <typename T1, typename T2>
concat_iterator<T1,T2> concat_end( T1 b1, T1 e1, T2 b2, T2 e2 )
{
   return concat_iterator<T1,T2>(e1,e1,e2,e2);
}

Now you could use:

 class X {
 public:
    template <typename Iter, typename Iter2>
    X(Iter b1, Iter e1, Iter2 b2, Iter2 e2 ) 
      : mVec( concat_begin(b1,e1,b2,e2), concat_end(b1,e1,b2,e2) ) 
    {}

  private:
    vector<Y> const mVec;
};

or (I have just thought of it) you don't need to redeclare your constructor. Make your caller use the helper functions:

X x( concat_begin(b1,e1,b2,e2), concat_end(b1,e1,b2,e2) );

I have not checked the code, just typed it here off the top of my head. It could compile or it could not, it could work or not... but you can take this as a start point.

David Rodríguez - dribeas
  • 204,818
  • 23
  • 294
  • 489
2

It would probably be best to drop const (why would you insist on it anyway?).

Otherwise, you have to build a concatenating iterator. It is quite a lot of code, see this thread for more.

avakar
  • 32,009
  • 9
  • 68
  • 103
  • In my case, the vector member variable shouldn't change after the instance has been constructed. Making it const helps the compiler help me guarantee that. – SCFrench Apr 16 '09 at 17:39
  • Well, given the amount of code necessary to do the concatenation, it is more likely your code will have a bug if you keep the const. – avakar Apr 16 '09 at 17:53
  • SCFrench, isn't it safe enough that the X::mvec won't change after the X has been constructed? – veefu Apr 16 '09 at 18:51
  • I want the compiler to guarantee that X::mVec won't be changed by any of X's methods (in case I mess up and accidentally try to modify it when I shouldn't). If I remove the const on the declaration of mVec, then any non-const method of X can muck with mVec. – SCFrench Apr 16 '09 at 21:12
2

One of the best or worst features of C++, depending on your viewpoint, is that you can abuse it when necessary to get the job done. In this case, const_cast is the victim:

template <typename Iter1, typename Iter2>
X(Iter1 begin1, Iter1 end1, Iter2 begin2, Iter2 end2) : mVec(begin1, end1) {
    const_cast<vector<Y>&>(mVec).insert(mVec.end(), begin2, end2);
}

I might have some of the details wrong, I didn't try to compile this. But it should give you the idea.

Mark Ransom
  • 299,747
  • 42
  • 398
  • 622
  • I was working on a rather complex iterator wrapper that would take both ranges... this is much simpler, even if... const_cast ouch! – David Rodríguez - dribeas Apr 16 '09 at 17:52
  • This is actually undefined behavior. You shouldn't remove const from objects that were defined const. You can remove const from const references bound to a non-const object, though. – avakar Apr 16 '09 at 17:52
  • 1
    Undefined doesn't always mean unpredictable. As I said, it's definitely abuse. – Mark Ransom Apr 16 '09 at 17:58
  • 1
    Since I spent a fair bit of time trying to figure out the compiler error I was getting from the example, I'll mention it here: Don't forget the second const_cast within the insert ".insert(const_cast&>(mvec).end(), begin2, end2):" – veefu Apr 16 '09 at 18:46
  • @Mark Ransom: "Undefined doesn't always mean unpredictable" Thats an oxy moron if ever I heard one. Why on earth do you think that it is predictable? This is a really really really bad idea. – Martin York Apr 17 '09 at 00:01
1

Your static method might not be as bad as you think, depending on the optimization your compiler does. And in C++0x, move constructors will remove any copying that is currently taking place.

In the meantime go with a wrapper iterator. The code is not likely to be as bad as the thread avakar links to, since you only need to implement an input iterator.

Eclipse
  • 44,851
  • 20
  • 112
  • 171
1

1) I would like to preserve the const on mVec, so that it is considered constant throughout the other methods of X.

  • This is a curious use of const on a member variable. And it defies good design. By definition, construction is a process which requires the object to change.

  • As for your requirement to keep the object non-modifiable -- use proper encapsulation. You should use const-member functions to expose any functionality based on your mVec for the clients of your class.

2) I would like to avoid unnecessary copies if at all possible. That is, one solution is to have a static method that constructs a non-const temporary to range 1, inserts range 2 and returns it, and then define the concatenating constructor to

You should be looking at move-constructors and r-value references in general (a promised goal of C++0x). Read this article.

dirkgently
  • 108,024
  • 16
  • 131
  • 187
  • I don't understand your first bullet. In my case, once the object is created, the vector is supposed to stay unmodified. Wouldn't marking it const help enforce that? What would be a non-curious use of const on a member variable? – SCFrench Apr 16 '09 at 21:09
  • I haven't seen much use of a `const` member variable when it was not static. What you want is a readonly member and there is no construct in C++ to do it the way you want. Your best bet is to use const accessors rather than invoking UB by modification due to const_casts. – dirkgently Apr 17 '09 at 04:34