patterncppMinor
Template vector class
Viewed 0 times
templatevectorclass
Problem
I am trying to implement a vector-like container of my own (just to gain a better understanding of how
Although I've succeeded in implementing most of the interface (only the parts I use the most), I'm still uncertain whether:
Please review it to see if it can be improved. Any suggestions related to my commenting style are also welcome!
```
size_t nearest_power_of_2(size_t n) // Return the nearest( and strictly greater than ) number to "n" which is a power of 2..
{
int count=0;
while(n)
{
n>>=1;
count++;
}
return 1ULL
class myvector
{
T* vector_pointer; // A pointer pointing to the start of the dynamic array...
size_t vector_size,vector_capacity;
public:
myvector():vector_pointer(NULL),vector_size(0),vector_capacity(0) {} // Default Constructor
myvector(const myvector& other):vector_pointer(NULL),vector_size(0),vector_capacity(0) // Copy Constructor
{
vector_pointer=new T[other.capacity()];
memcpy(vector_pointer,&other[0],sizeof(T)*other.size());
vector_size=other.vector_size;
vector_capacity=other.vector_capacity;
}
~myvector() // Destructor
{
delete[] vector_pointer;
}
myvector& operator =(myvector other) // Assignment operator
{
swap(*this,other);
return *this;
}
void resize(const size_t &newsize) // Change the size of the vector exactly to "newsize"..
{
if(newsizevector_capacity)
{
reserve(newsize);
}
vector_size=newsize;
}
void reserve(size_t newcapacity) // Change the capacity of the vector to be at least equal to "newcapacity"
{
newcapacity=nearest_power_of_2(newcapacity); // Keep the capacity of the vector as a power of 2 to av
std::vector works under the hood). I've been using this as a reference. Although I've succeeded in implementing most of the interface (only the parts I use the most), I'm still uncertain whether:
- There are no memory leaks.
- All the routines associate with
std::vectorwith respect to performance and space-usage.
Please review it to see if it can be improved. Any suggestions related to my commenting style are also welcome!
```
size_t nearest_power_of_2(size_t n) // Return the nearest( and strictly greater than ) number to "n" which is a power of 2..
{
int count=0;
while(n)
{
n>>=1;
count++;
}
return 1ULL
class myvector
{
T* vector_pointer; // A pointer pointing to the start of the dynamic array...
size_t vector_size,vector_capacity;
public:
myvector():vector_pointer(NULL),vector_size(0),vector_capacity(0) {} // Default Constructor
myvector(const myvector& other):vector_pointer(NULL),vector_size(0),vector_capacity(0) // Copy Constructor
{
vector_pointer=new T[other.capacity()];
memcpy(vector_pointer,&other[0],sizeof(T)*other.size());
vector_size=other.vector_size;
vector_capacity=other.vector_capacity;
}
~myvector() // Destructor
{
delete[] vector_pointer;
}
myvector& operator =(myvector other) // Assignment operator
{
swap(*this,other);
return *this;
}
void resize(const size_t &newsize) // Change the size of the vector exactly to "newsize"..
{
if(newsizevector_capacity)
{
reserve(newsize);
}
vector_size=newsize;
}
void reserve(size_t newcapacity) // Change the capacity of the vector to be at least equal to "newcapacity"
{
newcapacity=nearest_power_of_2(newcapacity); // Keep the capacity of the vector as a power of 2 to av
Solution
You can do this with maths rather than a loop:
Couple of issues with the copy constructor.
Similar issues with resize()
Making the size smaller is easier. You don't need to re-allocate. You can just reduce the size value and manually call the destructor on all the objects that don't exist anymore.
Same but slightly different in reserve()
Your push back is very inefficient:
The pop back needs work
More times when destructor needs to be called.
Fix your output operator:
For loops like this use the standard algorithms:
Personally I don't see the point. I can achieve the same effect in one line:
Simple Vector:
``
size_t nearest_power_of_2(size_t n) // Return the nearest( and strictly greater than ) number to "n" which is a power of 2..Couple of issues with the copy constructor.
myvector(const myvector& other):vector_pointer(NULL),vector_size(0),vector_capacity(0) // Copy Constructor
{
// Don't assign to this object until you
// know that no exceptions can be thrown.
// This means all memory allocation and
// object copying must be completed first.
vector_pointer=new T[other.capacity()];
// You can use this for POD types.
// But any non POD types must be copied using their copy constructor.
memcpy(vector_pointer,&other[0],sizeof(T)*other.size());
vector_size=other.vector_size;
vector_capacity=other.vector_capacity;
}Similar issues with resize()
// You can use this for POD types.
// But any non POD types must be copied using their copy constructor.
memcpy(temp,vector_pointer,sizeof(T)*newsize);Making the size smaller is easier. You don't need to re-allocate. You can just reduce the size value and manually call the destructor on all the objects that don't exist anymore.
Same but slightly different in reserve()
// The trouble with use T as the underlying type.
// As that this call will call the constructor on all objects
// upto `capacity`. You don't want that you only want to call
// the constructor on values upto `newcapacity`
T* temp=new T[newcapacity];
// You can use this for POD types.
// But any non POD types must be copied using their copy constructor.
memcpy(temp,vector_pointer,sizeof(T)*vector_capacity);Your push back is very inefficient:
void push_back(const T &val) // Add a new element of value "val" at the end of the vector..
{
if(vector_capacity<=vector_size)
// As noted calls copy constructor
reserve(vector_capacity);
// Now using the assignment operator.
// To copy over object that you just fully initialized.
vector_pointer[vector_size++]=val;
// A better solution is not to construct in the copy constructor.
// Then here use placement new to create the object in place
}The pop back needs work
void pop_back() // Remove the last element of the vector..
{
if(vector_size)
--vector_size;
// RAII is an important concept in C++
// When you pop something out of your vector it should no longer exist.
// This means you need to call the destructor for the object.
// Here that means manually calling the destructor.
} // Doesn't actually deallocate the element block (for performance)..More times when destructor needs to be called.
void erase(const size_t& pos) // Erase the element at "pos"..
void clear()
// For the elements passed the end of the valid vector (vector_size)
// You need to call the destructors of these elements as they no longer exist.Fix your output operator:
templatestd::ostream& operator &a) // Overloaded output operator to display the contents of a vector..
{
for(size_t i=0; i<a.size(); i++)
{
// Should be `out`
std::cout<<a[i]<<" ";
}
return out;
}For loops like this use the standard algorithms:
Personally I don't see the point. I can achieve the same effect in one line:
std::copy(a.begin(), a.end(), std::ostream_iterator(out, " "));Simple Vector:
``
template
class LokiVector
{
std::size_t size;
std::size_t capacity;
std::unique_ptr data; // Use char so we don't worry about
// constructor/destructor issues.
void init(T const& value)
{
public:
LokiVector(std::size_t startSize = 0, T const& defaultValue = T())
: size(startSize)
, capacity(std::min(startSize, 16))
, data(new char[sizeof(T) * capacity])
{
T* localData = reinterpret_cast(data.get());
for(int loop = 0;loop (data.get());
for(int loop = 0;loop (data.get());
for(int loop = 0;loop (data.get());
new (&localData[size]) T(value);
++size;
}
void pop_back()
{
if (size > 0)
{
--size;
T* localData = reinterpret_cast(data.get());
localData[size].~T();
}
}
void reserve(std::size_t min)
{
if (min tmp_data(new char[sizeof(T) * tmp_capacity);
T* localData = reinterpret_cast(data.get());
T* tmpLocal = reinterpret_cast(tmp_data.get());
for(int loop = 0;loop (data.get());
return localData[index];
}
T const& operator[] (std::size_t index) const
{
return const_cast(*this)[index];
}
}
`Code Snippets
size_t nearest_power_of_2(size_t n) // Return the nearest( and strictly greater than ) number to "n" which is a power of 2..myvector(const myvector& other):vector_pointer(NULL),vector_size(0),vector_capacity(0) // Copy Constructor
{
// Don't assign to this object until you
// know that no exceptions can be thrown.
// This means all memory allocation and
// object copying must be completed first.
vector_pointer=new T[other.capacity()];
// You can use this for POD types.
// But any non POD types must be copied using their copy constructor.
memcpy(vector_pointer,&other[0],sizeof(T)*other.size());
vector_size=other.vector_size;
vector_capacity=other.vector_capacity;
}// You can use this for POD types.
// But any non POD types must be copied using their copy constructor.
memcpy(temp,vector_pointer,sizeof(T)*newsize);// The trouble with use T as the underlying type.
// As that this call will call the constructor on all objects
// upto `capacity`. You don't want that you only want to call
// the constructor on values upto `newcapacity`
T* temp=new T[newcapacity];
// You can use this for POD types.
// But any non POD types must be copied using their copy constructor.
memcpy(temp,vector_pointer,sizeof(T)*vector_capacity);void push_back(const T &val) // Add a new element of value "val" at the end of the vector..
{
if(vector_capacity<=vector_size)
// As noted calls copy constructor
reserve(vector_capacity);
// Now using the assignment operator.
// To copy over object that you just fully initialized.
vector_pointer[vector_size++]=val;
// A better solution is not to construct in the copy constructor.
// Then here use placement new to create the object in place
}Context
StackExchange Code Review Q#29331, answer score: 8
Revisions (0)
No revisions yet.