0

For customized use, I have inherited std::vector to a custom class Vector. For my requirement this public inheritance is ok.

One intention is to avoid making copies of a vector array multiple times, so I decided this custom class to be 'ownership' based.

Vector<A> vA1(100); // vA1 is allocated A[100]
Vector<A> vA2 = vA1; // vA2 refers to original A[100] and ...
                     // ... vA1 is now blank which is expected

This is how it's implemented for C++03:

template<typename T>
struct Vector : std::vector<T>
{
  // ... other constructors

  Vector (const Vector &copy) // copy constructor
  {
    this->swap(const_cast<Vector&>(copy)); // <---- line of interest
  }
  // 'operator =' is TBD (either same as above or unimplemented)
};

Am I breaking any language rule/feature/convention ? Is there anything bad in this code ?

Edit: I have added my new approach in form of answer below (Here is its working demo).

Community
  • 1
  • 1
iammilind
  • 68,093
  • 33
  • 169
  • 336
  • 2
    If you're going to overload `operator=`, it should have the same semantic meaning as `=`. Yours doesn't. Ditto for your copy constructor. – David Schwartz Sep 30 '12 at 16:56
  • @DavidSchwartz, yup, I am yet to decide for `operator =` and updated that in question. For now, main concern is about copying. – iammilind Sep 30 '12 at 16:59
  • Why `const Vector &copy` and the cast? Why not just `Vector (Vector& victim)`? – David Schwartz Sep 30 '12 at 17:00
  • @DavidSchwartz, that's a good question. For conventional way of having copy constructor, I went for `const`. I slightly remember that few years back g++ compiler din't support non-const way of copying. If `Vector(Vector&)` is universally accepted then I will be more than happy. – iammilind Sep 30 '12 at 17:01
  • 5
    Should you be using `std::auto_ptr` instead? It has nearly the same semantics you're trying to accomplish here. – Thomas Sep 30 '12 at 17:02
  • 2
    Passing such objects by value could potentially hide a lot of bugs. – Ivan Vergiliev Sep 30 '12 at 17:03
  • @Thomas, I think `auto_ptr` are considered [deprecated](http://stackoverflow.com/questions/6293075/is-c11-c0x-a-complete-super-set-of-c03) in C++11. Though I am not coding for C++11, I want to follow its conventions. – iammilind Sep 30 '12 at 17:04
  • 2
    It's deprecated in C++11 because C++11 has move semantics and can therefore provide `std::unique_ptr`. Since C++03 doesn't have either, there's nothing wrong with `auto_ptr`. – Thomas Sep 30 '12 at 17:07
  • Anyway, the main reason that `unique_ptr` is superior to `auto_ptr` is that it doesn't surprisingly modify the source of a copy (only the source of a move). Since you're deliberately writing code to surprisingly modify the source of a copy, the reasons for the deprecation don't apply. – Steve Jessop Sep 30 '12 at 17:11

4 Answers4

4

I think what you are looking for are move semantics.

Vector<A> vA1(100); // vA1 is allocated A[100]
Vector<A> vA2 = std::move(vA1); // vA2 refers to original A[100] and ...
                 // ... vA1 is now blank which is expected

If your compiler doesn't have move semantics support I would suggest you use smart pointers or take a look at boost::move which can emulate move semantics in pre C++11 compilers..

std::shared_ptr<Vector<A>> vA1(new Vector<A>(100)); // vA1 is allocated A[100]
std::shared_ptr<Vector<A>> vA2 = vA1; // vA2 refers to original A[100] and ...
vA1.reset();
                 // ... vA1 is now blank which is expected

or even simpler:

Vector<A> vA1(100); // vA1 is allocated A[100]
Vector<A> vA2;
vA1.swap(vA2);// vA2 refers to original A[100] and ...
                 // ... vA1 is now blank which is expected
ronag
  • 49,529
  • 25
  • 126
  • 221
3

You should really have a look at using C++11: all the standard containers are movable and are thus resembling your semantics in many cases, namely when it is known that the ownership can be transferred. More precisely, containers get moved rather than copied when returning objects from function or when using a temporary to construct or assign an object. Trying to replicate this mechanism in some way will almost certainly yield unexpected behavior in may cases.

In C++11 the movable semantics are integrated into the language and functions, including constructors and assignments, can be overloaded to behave different depending on whether the object is movable or not.

ronag
  • 49,529
  • 25
  • 126
  • 221
Dietmar Kühl
  • 150,225
  • 13
  • 225
  • 380
2

Your code breaks the conventions that support const-correctness:

const Vector<int> foo(12);
const Vector<int> bar(foo); // UB.
some_function_that_takes_Vector_by_value(bar); // UB.

You might say, "but nobody will ever create a const instance of Vector, so the UB will never happen". In which case, you could at least make your copy ctor take a non-const reference in order to follow the convention that functions shouldn't modify objects taken by const& parameters.

Even then, personally I'd prefer to require the user to "switch on" speed by explicitly swapping, than to create traps like this for them. I'd also prefer to have no copy ctor at all, than one which behaves unlike a normal copy. If people are going to have to bear in mind that Vector cannot actually be copied, then they shouldn't use copy syntax to not-copy it.

The issue of unnecessary copies in C++03 isn't a new one and IMO it doesn't require a novel solution. Many C++ programmers already know how to take advantage of swap and copy elision, and those who don't know how to avoid unnecessary copies are better off learning to do it the normal way, than learning to do it a unique way demanded by your one specific class.

Steve Jessop
  • 273,490
  • 39
  • 460
  • 699
1

With the insight from comments/answers, I realized that my current approach is not a good idea to deal with the things.
I have come up with a changed designed where I try to simulate the "move" operation using swap() method (consider them interchangeable in this context).

For any class or container, which wants to make faster copying (i.e. move by swapping) should inherit below class (directly copy pasting from the real file):

  /*
   * An empty CRTP base class for the classes which are allowing move sematics (effectively swap)
   * A given class `X` must implement `void swap(X&);` method for the movement (i.e. content swapping)
   * For example, `X` is deriving this class (i.e. `class X : public Movable<X> ...`)
   * ... below is an illustration for how to transfer contents of an `X` object to another
   * `X o1; ... X o2(o1.move()); // contents of o1 is now moved to o2`
   */
  template<typename Derived>
  class Movable
  {
    /*
     * Empty constructor for normal behavior
     */
    public: Movable ()
            {}

    /*
     * Copy constructor which does nothing; without this compiler errors out !
     */
    public: Movable (const Movable &copy)
            {}

    /*
     * Move constructor which will effectively call `Derived::swap()` method
     * After this, the contents of the object will be moved to each other
     * For syntactic sugar, it has been made `explicit`
     */
    public: explicit Movable (Movable &orig)
            {
              (*this)(orig);
            }

    /*
     * Moving while Not initializing object, one need to use `operator ()` and not `operator =`
     * If `operator =` is used then simply `move()` part will not happen; i.e. below 2 are same:
     * `obj1 = obj2.move();` and `obj1 = obj2;`
     */
    public: void operator () (Movable &orig)
            {
              static_cast<Derived*>(this)->swap(static_cast<Derived&>(orig));
            }
    /*
     * This method is called from `Derived` class when move sematics is intended
     */
    public: Movable& move ()
            {
              return *this;
            }
  };

Here how it's supposed to be deployed:

template<typename T>
struct Vector : std::vector<T>,
                Movable<Vector<T> >  // inherit as CRTP base
{
  // ... other methods
  typedef Movable<Vector<T> > Movable_;
  Vector (Movable_ &orig) : Movable_(orig) {}  // Declare a constructor
};

Here how it's supposed to be used:

Vector<A> vA1(100); // vA1 is allocated A[100]
Vector<A> vA2 = vA1; // normal copying (no change)
vA2 = vA1; // normal assignment (no change)
Vector<A> vA3(vA1.move()); // <------- "moves" contents from 'vA1' to 'vA3'
vA1(vA2.move()); // <------- "moves" contents from 'vA2' to 'vA1'
vA2 = vA3.move(); // normal copying from 'vA3' to 'vA2' ('vA3' unaffected)
iammilind
  • 68,093
  • 33
  • 169
  • 336