• Advertisement
Sign in to follow this  

inheriting from std::vector

This topic is 4251 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

I'm trying to make this class that's inherited from std::vector in C++. However, it gives an error that I don't understand. For some reason, it doesn't want me to use "size()", while size really is part of the class I inherited from and public, so it really should be accessible without strange error!
#include <vector>

template<typename T>
class pvector : public std::vector<T>
{
     public:
     ~pvector() { for(int i = 0; i < size(); i++) delete this; }
};

/*
errors:

pvector.h: In destructor 'pvector<T>::~pvector()':
pvector.h:7: error: there are no arguments to 'size' that depend on a template parameter, so a declaration of 'size' must be available
pvector.h:7: error: (if you use '-fpermissive', G++ will accept your code, but allowing the use of an undeclared name is deprecated)
*/


I don't plan on using -fpermissive. What's wrong and how do I fix it? Thanks.

Share this post


Link to post
Share on other sites
Advertisement

#include <vector>

template<typename T>
class pvector : public std::vector<T>
{
typedef pvector<T> this_type;
public:
~pvector() { for(int i = 0; i < this_type::size(); i++) delete this_type::operator[](i); }
};



You must specify the concrete type of the vector who's methods you want to invoke, since both size() and operator[] lack the required template arguments as the destructor is declared without having one (or something along the lines of that[lol]).

HTH,
Pat.

Share this post


Link to post
Share on other sites
std::vector does not have a virtual destructor, which is a good indication that you shouldn'y be trying to derive from it. Consider instead making a std::vector a member variable of your class.

Share this post


Link to post
Share on other sites
cool, it works now, it also works with (*this)[] instead of this_type::operator[](i), really strange, I wonder why it works for operator[] but not for size()

Also, I don't know if this_type is something that exists for some compilers, but it didn't exist in mine so I used std::vector<T>::size()

Thanks to this I can do proper OO now without having to delete stuff myself and without boost :)

#ifndef pvector_h
#define pvector_h

#include <vector>

template<typename T>
class pvector : public std::vector<T>
{
public:
~pvector() { for(int i = 0; i < std::vector<T>::size(); i++) delete (*this); }
};

#endif

Share this post


Link to post
Share on other sites
Quote:
Original post by Lode
cool, it works now, it also works with (*this)[] instead of this_type::operator[](i)

Thanks to this I can do proper OO now without having to delete stuff myself and without boost :)


Heed SiCranes warning.

It would be preferable to implement your class using composition. Your current code risks leaking memory for pvectors, precisely that which the class was designed to overcome.

Also note that your class doesnt allow you to specify a custom allocator for the vector.

Share this post


Link to post
Share on other sites
Oh...

I was actually first planning to make a new class with an std::vector that's a member of it. However, I'd have to type over ALL functions and operators of std::vector for it, because I want them all to be part of this class as well. So that's why someone suggested me on an irc channel to inherit from std::vector instead... I guess he didn't know that it was dangerous either.

But if the only problem could be polymorphism: I'm not planning to polymorph vectors.

EDIT:

I tested it with valgrind and it gave 0 errors, 0 leaks, for this code:

#include "pvector.h"
#include <iostream>

class A
{
public:

virtual int a() { return 1; }
virtual ~A(){}
};

class B : public A
{
public:

int a() { return 2; }
};

int main()
{
pvector<A*> v;

v.push_back(new A());

v.push_back(new B());

std::cout << v[0]->a() << "\n" << v[1]->a() << "\n";

return 0;
}


valgrind output:

1
2
==6021==
==6021== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 20 from 1)
==6021== malloc/free: in use at exit: 0 bytes in 0 blocks.
==6021== malloc/free: 5 allocs, 5 frees, 22 bytes allocated.
==6021== For counts of detected errors, rerun with: -v
==6021== All heap blocks were freed -- no leaks are possible.

Share this post


Link to post
Share on other sites
Privately inheriting from std::vector is safe, and you can expose the functions you want with the using keyword.


To view why your current code in unsafe, try this instead:

#include "pvector.h"
#include <iostream>

class A
{
public:

virtual int a() { return 1; }
virtual ~A(){}
};

class B : public A
{
public:

int a() { return 2; }
};

int main()
{
std::vector<A*>* v = new pvector<A*>();

v->push_back(new A());

v->push_back(new B());

std::cout << (*v)[0]->a() << "\n" << (*v)[1]->a() << "\n";

delete v;

return 0;
}



So, while your correct that non-polymorphic use of your class is safe, your class will compile without any errors if it is used unsafely.

Share this post


Link to post
Share on other sites
Quote:
Original post by Nitage
Privately inheriting from std::vector is safe, and you can expose the functions you want with the using keyword.


To view why your current code in unsafe, try this instead:
*** Source Snippet Removed ***

So, while your correct that non-polymorphic use of your class is safe, your class will compile without any errors if it is used unsafely.


But the thing is, it's not supposed to be used polymorphically!

I now added a comment like in the header saying that one should only use it non-polymorphically, and it should only contain pointers created with new.

So... if it'll only be used non-polymorphically, can it then be considered ok, or is it still considered very bad by programmers because it has the ability to be used in a bad way?

Also, can you please explain a bit more about the private inheriting and the using keyword? I've never seen it and have no idea where this using keyword has to be typed and how it works.

Thanks.

Share this post


Link to post
Share on other sites
Here's an example:

class Base
{
protected:
void Func1(){cout << "Func1" << endl;}
void Func2(){cout << "Func2" << endl;}
};

class Foo : Base
{
public:
using Base::Func1;
};



Foo foo;

foo.Func1(); //okay
foo.Func2(); //can't do that


Share this post


Link to post
Share on other sites
Quote:
Original post by Lode
I now added a comment like in the header saying that one should only use it non-polymorphically


You should rather be enforcing such usage (at compile time, if possible).

Not necessarily what you want, but here's a dirty hack to prevent it from being used "uncorrectly". (as a side note: it compiles fine on vs7.1)

template<typename T>
class pvector : public std::vector<T>
{
public:
~pvector() { for(size_t i=0; i<size(); i++) delete (*this); }
private:
void* operator new(size_t size);
void* operator new[](size_t size);
};



Another thing:
Watch that you won't be able to use templated functions specialized for std::vector, like std::swap. Instead, pvector will be delegated to non-specialized version (and in case of swap, this would crash the program).

Share this post


Link to post
Share on other sites
Allright, I decided I'll privatly inherit from std::vector then, and only use "using" on functions of std::vector that are actually needed, to avoid all possible problems described and any other possible things that could screw it up.

Thanks for the help people!

I learned a lot of new things about C++ today :)

Share this post


Link to post
Share on other sites
Allright, here's the result! Do you think it's fine now?

The inheritance is now private, and now it'll also do delete on elements if you do clear(), erase(), pop_back() and resize()!

I didn't do "using" on the function "swap" because of the reason posted by deffer.

I drew inspiration from /usr/include/c++/4.0.3/bits/stl_vector.h to use the same syntax as them for some of the functions.


#ifndef pvector_h
#define pvector_h

#include <vector>

/*
pvector by Lode Vandevenne
A pvector works like an std::vector but it'll use the delete operator on all elements when it's destructed.
The elements should always be pointers created with new.
Only non-polymorphic use is allowed (because it's inherited from std::vector which has a non-virtual dtor).
*/

template<typename T>
class pvector : std::vector<T>
{
public:
~pvector()
{
for(typename std::vector<T>::size_type i = 0; i < std::vector<T>::size(); i++) delete (*this);
}

void pop_back()
{
delete (*this)[std::vector<T>::size() - 1];
std::vector<T>::pop_back();
}

void resize(typename std::vector<T>::size_type __new_size)
{
for(int i = __new_size; i < std::vector<T>::size(); i++) delete (*this);
std::vector<T>::resize(__new_size);
}

void erase(typename std::vector<T>::iterator __position)
{
delete *__position;

std::vector<T>::erase(__position);
}

void erase(typename std::vector<T>::iterator __first, typename std::vector<T>::iterator __last)
{
for(typename std::vector<T>::iterator __position = __first; __position != __last; __position++)
{
delete *__position;
}

std::vector<T>::erase(__first,__last );
}

void clear()
{
erase(begin(), end());
}

using std::vector<T>::size;
using std::vector<T>::operator[];
using std::vector<T>::push_back;
using std::vector<T>::begin;
using std::vector<T>::end;
using std::vector<T>::rbegin;
using std::vector<T>::rend;
using std::vector<T>::at;
using std::vector<T>::back;
using std::vector<T>::capacity;
using std::vector<T>::empty;
using std::vector<T>::front;
using std::vector<T>::max_size;
using std::vector<T>::reserve;
using std::vector<T>::insert;
//using std::vector<T>::swap; //No!
};

#endif

Share this post


Link to post
Share on other sites
Quote:
Original post by phantom
Just to throw this out there, but I wouldn't be surprised if the Boost pointer containters can do just what you are after.

I'd consider this a learning exercise and then use Boost for future projects [smile]


It was learning exercise! It was inspired by the ptr_vector by the way, when I heard about its feature I immediatly knew I wanted it.

I also like having my code independent from boost, so I'm going to keep using this pvector from now on. Those 73 lines of code are the only boost-feature I need so I prefer using this over the huge thing that boost is. Really, Boost is all great and stuff, but, it's huge!

I don't need a list version. I don't really need to make pointers to the pointers kept by this type of container for now. But if I'd for some reason need a list later I'll make plist too.

Share this post


Link to post
Share on other sites
The semantics of your class are completely screwy. By exposing the pointers directly to the clients of your class you basically forgo any protection that your class provides. Also, consider the effects if insert(), push_back() or a similar operation throws an exception: unless every call you make that will potentially enlarge the vector explicitly wraps the calls with try/catch blocks, you're going to have a memory leak. This is not kosher for an operation that implicitly assumes ownership of the pointed to object. Go look up the boost pointer container documentation and look at their interface; there's a reason why it's made the way it is.

Share this post


Link to post
Share on other sites
Darn programming is hard, it's impossible to do something in a straightforward way :(

I'm getting discouraged but at the same time I find it interesting that I'm finally starting to see why so many complex pieces of code are to be found in the STL and Boost.

Share this post


Link to post
Share on other sites
It depends on your definition of "straightforward".

boost::ptr_vector<MyClass> my_vector; // seems pretty straightforward to me

Share this post


Link to post
Share on other sites
Quote:
Original post by Lode
Darn programming is hard, it's impossible to do something in a straightforward way :(


Writing good library components is probably the one of the hardest programming task there is. Making sure it is safe yet easy to use is difficult and takes significant effort. Sacrificing either just because it saves on typing is not the way to write great code.

Quote:
I'm getting discouraged but at the same time I find it interesting that I'm finally starting to see why so many complex pieces of code are to be found in the STL and Boost.


They've gone through the trouble of getting it right, might as well not duplicate the effort and focus on the things they don't provide.

Quote:
I also like having my code independent from boost, so I'm going to keep using this pvector from now on. Those 73 lines of code are the only boost-feature I need so I prefer using this over the huge thing that boost is. Really, Boost is all great and stuff, but, it's huge!


And so is the Windows API. I don't really see where the library's size is a problem. It's not like it all gets added to your program. Once you have Boost installed on your machine you don't really have to worry about it anymore.

Share this post


Link to post
Share on other sites
Quote:
Original post by Lode
I also like having my code independent from boost, so I'm going to keep using this pvector from now on. Those 73 lines of code are the only boost-feature I need so I prefer using this over the huge thing that boost is. Really, Boost is all great and stuff, but, it's huge!


ptr_vector is a header only library. and if you're really that short on space that you don't want the rest of the libraries on your hard drive, use bcp to extract the files you need for ptr_vector.

Share this post


Link to post
Share on other sites
Allright, another hopefully improved version.

Now, you can't access the pointers anymore, note how the template type is T but the std::vector inherited from is of type T*, and how operator[] returns the element as reference and not the pointer.

The resize function is removed, you're supposed to push_back elements one by one, with the new operator used.

And finally, the push_back function will delete the element if memory is full (but I commented out two things that I didn't know what they do, it comes from the stl_vector.h file).

EDIT: and, now it also checks if the pointer isn't a null pointer, before deleting.
EDIT2: this test removed again, see next post by Fruny

#ifndef pvector_h
#define pvector_h

#include <vector>

/*
A pvector works like an std::vector but is for pointers to polymorphed objects.
It'll use the delete operator on all elements that get destroyed.
It returns the elements as references instead of pointers.
The elements should always be pointers created with new.
*/

template<typename T>
class pvector : std::vector<T*>
{
public:
~pvector()
{
for(typename std::vector<T*>::size_type i = 0; i < std::vector<T*>::size(); i++) delete &(*this);
}

void pop_back()
{
delete (*this)[std::vector<T*>::size() - 1];
std::vector<T*>::pop_back();
}

void erase(typename std::vector<T*>::iterator __position)
{
delete *__position;

std::vector<T*>::erase(__position);
}

void erase(typename std::vector<T*>::iterator __first, typename std::vector<T*>::iterator __last)
{
for(typename std::vector<T*>::iterator __position = __first; __position != __last; __position++)
{
delete *__position;
}

std::vector<T*>::erase(__first,__last );
}

void clear()
{
erase(begin(), end());
}

T& operator[](typename std::vector<T*>::size_type __n)
{
return *(std::vector<T*>::operator[](__n));
}

void push_back(const typename std::vector<T*>::value_type& __x)
{
if (this->_M_impl._M_finish != this->_M_impl._M_end_of_storage)
{
delete __x; //just delete the thing already, seriously, normally memory shouldn't go full
//this->_M_impl.construct(this->_M_impl._M_finish, __x);
//++this->_M_impl._M_finish;
}
else
{
std::vector<T*>::_M_insert_aux(end(), __x);
}
}

using std::vector<T*>::size;
using std::vector<T*>::begin;
using std::vector<T*>::end;
using std::vector<T*>::rbegin;
using std::vector<T*>::rend;
using std::vector<T*>::at;
using std::vector<T*>::back;
using std::vector<T*>::capacity;
using std::vector<T*>::empty;
using std::vector<T*>::front;
using std::vector<T*>::max_size;
using std::vector<T*>::reserve;
//using std::vector<T*>::resize; //you're supposed to push_back "new" objects
//using std::vector<T*>::insert; //disabled
//using std::vector<T*>::swap; //No!
};

#endif



[Edited by - Lode on July 2, 2006 4:12:54 PM]

Share this post


Link to post
Share on other sites
Quote:
Original post by Lode
EDIT: and, now it also checks if the pointer isn't a null pointer, before deleting.


Deleting a null pointer is not an error. The test is unnecessary.

I note that your iterators will still return pointers.

You are also missing the const versions of your operators.

You should not name your parameters __n, __first, __last: standard library components are allowed to, you are not, precisely because such names are reserved for the library and compiler usage.

Share this post


Link to post
Share on other sites
Quote:
if (this->_M_impl._M_finish != this->_M_impl._M_end_of_storage)
{
delete __x; //just delete the thing already, seriously, normally memory shouldn't go full
//this->_M_impl.construct(this->_M_impl._M_finish, __x);
//++this->_M_impl._M_finish;
}


Are you serious?

Unless your vector's size() reaches it's capacity(), you just delete the object you were trying to add?

That's completely broken.

Remember how vector allocates an array and, if that array gets filled, it'll allocate a larger one and copy things over? Well... that's what this test is for. construct constructs the object in the existing memory using the vector's Allocator (which may end up just doing a placement new).

Edit: If → Unless.

Share this post


Link to post
Share on other sites
Your push_back() probably should be more like:

void push_back(T * ptr) {
try {
std::vector<T *>::push_back(ptr);
} catch (...) {
delete ptr;
throw;
}
}

Relying on internal details like your version is a good indication that you're doing something bad.

Share this post


Link to post
Share on other sites
The thing I did with push_back was a big mistake...

Ok, now with the try-catch block, and a design-decision: no more iterators.

EDIT: fixed erase functions, or they would have been pretty useless without iterators

#ifndef pvector_h
#define pvector_h

#include <vector>

/*
A pvector works like an std::vector but is for pointers to polymorphed objects.
It'll use the delete operator on all elements that get destroyed.
It returns the elements as references instead of pointers.
The elements should always be pointers created with new.
Not really safe so only for internal use by me.
*/

template<typename T>
class pvector : std::vector<T*>
{
public:
~pvector()
{
for(typename std::vector<T*>::size_type i = 0; i < std::vector<T*>::size(); i++) delete &(*this);
}

void pop_back()
{
delete (*this)[std::vector<T*>::size() - 1];
std::vector<T*>::pop_back();
}

void erase(typename std::vector<T*>::size_type position)
{
typename std::vector<T*>::iterator position_it = std::vector<T*>::begin() + position;
delete *position_it;

std::vector<T*>::erase(position_it);
}

void erase(typename std::vector<T*>::size_type first, typename std::vector<T*>::size_type last)
{
typename std::vector<T*>::iterator first_it = std::vector<T*>::begin() + first;
typename std::vector<T*>::iterator last_it = std::vector<T*>::begin() + last;
for(typename std::vector<T*>::iterator position_it = first_it; position_it != last_it; position_it++)
{
delete *position_it;
}

std::vector<T*>::erase(first_it, last_it);
}

void clear()
{
erase(0, size());
}

T& operator[](typename std::vector<T*>::size_type n)
{
return *(std::vector<T*>::operator[](n));
}

void push_back(T* x)
{
try
{
std::vector<T*>::push_back(x);
}
catch (...)
{
delete x;
throw;
}
}

using std::vector<T*>::size;
using std::vector<T*>::capacity;
using std::vector<T*>::empty;
using std::vector<T*>::max_size;
using std::vector<T*>::reserve;

};

#endif


But really, maybe you guys are right. Maybe I should take a look at Boost and its fully functional ptr_containers. Real shame that those ptr_containers won't make it into the next C++ standard though.

Share this post


Link to post
Share on other sites
Quote:
Original post by Lode
EDIT: fixed erase functions, or they would have been pretty useless without iterators


As you see, designing a container is really a all-or-nothing situation. If you start leaving bits out, you soon have nothing left.

And I won't even start to badger you about whether your container really is a Container, as per the C++ standard's requirements (hint: it isnt).

Quote:
But really, maybe you guys are right. Maybe I should take a look at Boost.


Word. Makes you appreciate how much work is involved, doesn't it? At the very least you should have a look at how they did implement their ptr_vector.

Quote:
Real shame that those ptr_containers won't make it into the next C++ standard though.


Maybe so, but I don't see why Boost being a 3rd party library should stop you from using it.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement