patterncppMinor
Implementation of std::vector class
Viewed 0 times
stdimplementationclassvector
Problem
I decided to implement the
I have not included every function in the
```
# include
// I included the below two headers for my tests
# include
# include
namespace mynamespace
{
template
class vector;
template
class vector_const_iterator;
template
class vector_iterator
{
public:
typedef vector_iterator this_t;
typedef typename Vec_T::value_type T;
friend Vec_T;
vector_iterator(T *ptr_, Vec_T const &cont)
: container(cont),
ptr(ptr_)
{
}
vector_iterator(this_t const &rhs)
: container(rhs.container),
ptr(rhs.ptr)
{
}
vector_iterator(vector_const_iterator const &rhs)
: container(rhs.container),
ptr(rhs.ptr)
{
}
~vector_iterator()
{
}
bool operator!=(this_t const &rhs) const
{
return (ptr != rhs.ptr);
}
bool operator!=(T *ptr_) const
{
return (ptr != ptr_);
}
bool operator==(this_t const &rhs) const
{
return (ptr == rhs.ptr);
}
bool operator==(T *ptr_) const
{
return (ptr == ptr_);
}
bool operator>(this_t const &rhs) const
{
return (ptr > rhs.ptr);
}
bool operator>(T *ptr_) const
{
return (ptr > ptr_);
}
bool operator>=(this_t const &rhs) const
{
return (ptr >= rhs.ptr);
}
bool operator>=(T *ptr_) const
{
return (ptr >= ptr_);
}
bool operat
std::vector class in C++, and was wondering if I had done everything correctly. When I mean 'correctly', I mean that the code is efficiently written and done properly. I have tested all the functions using various tests and they seem to output correct results.I have not included every function in the
std class: the main ones that I have not included are assign/insert and I have not implemented a reverse_iterator, although I have made my own iterator and const_iterator class.```
# include
// I included the below two headers for my tests
# include
# include
namespace mynamespace
{
template
class vector;
template
class vector_const_iterator;
template
class vector_iterator
{
public:
typedef vector_iterator this_t;
typedef typename Vec_T::value_type T;
friend Vec_T;
vector_iterator(T *ptr_, Vec_T const &cont)
: container(cont),
ptr(ptr_)
{
}
vector_iterator(this_t const &rhs)
: container(rhs.container),
ptr(rhs.ptr)
{
}
vector_iterator(vector_const_iterator const &rhs)
: container(rhs.container),
ptr(rhs.ptr)
{
}
~vector_iterator()
{
}
bool operator!=(this_t const &rhs) const
{
return (ptr != rhs.ptr);
}
bool operator!=(T *ptr_) const
{
return (ptr != ptr_);
}
bool operator==(this_t const &rhs) const
{
return (ptr == rhs.ptr);
}
bool operator==(T *ptr_) const
{
return (ptr == ptr_);
}
bool operator>(this_t const &rhs) const
{
return (ptr > rhs.ptr);
}
bool operator>(T *ptr_) const
{
return (ptr > ptr_);
}
bool operator>=(this_t const &rhs) const
{
return (ptr >= rhs.ptr);
}
bool operator>=(T *ptr_) const
{
return (ptr >= ptr_);
}
bool operat
Solution
There's a rather large amount of code here, so I'm just going to make some comments as I find things.
This looks like it might be a bug, did you mean to do
Additionally you probably want to check for overflow here too otherwise bad things will happen, make sure
Also I might be biased slightly because I've been using Python a lot lately but I particularly dislike declaring variables like this:
Putting one variable per line I find much nicer.
One thing I notice here is duplicated code for the operators in a few places. Specifically I'd implement as many operators as possible in terms of other operators. For example:
would become:
You can do this for a bunch of the operators, which will help the maintainability of the code.
Now while the following is not exactly duplicated:
It looks very similar. A comment explaining why these 2 seemingly similar functions exist and when to which version would be good. While most of the code seems self-explanatory the places that are less obvious would greatly benefit from some documentation.
EDIT as per the comments:
I might be inclined to rewrite the pointer validity check functions as follows:
This change makes it a lot clearer to the reader what is going on here.
void reallocate(unsigned size)
{
allocator_t al;
while (size == capacity())
{
size *= 2;
}This looks like it might be a bug, did you mean to do
while size <= capacity instead?Additionally you probably want to check for overflow here too otherwise bad things will happen, make sure
size*2 actually fits into an unsigned.Also I might be biased slightly because I've been using Python a lot lately but I particularly dislike declaring variables like this:
pointer new_memstart = al.allocate(size), new_seqend;Putting one variable per line I find much nicer.
One thing I notice here is duplicated code for the operators in a few places. Specifically I'd implement as many operators as possible in terms of other operators. For example:
bool operator==(this_t const &rhs) const
{
return (ptr == rhs.ptr);
}
bool operator!=(this_t const &rhs) const
{
return (ptr != rhs.ptr);
}would become:
bool operator==(this_t const &rhs) const
{
return (ptr == rhs.ptr);
}
bool operator!=(this_t const &rhs) const
{
return !operator==(rhs);
}You can do this for a bunch of the operators, which will help the maintainability of the code.
Now while the following is not exactly duplicated:
bool is_valid_ptr() const
{
return (ptr >= container.mem_start
&& ptr = container.mem_start
&& ptr <= container.seq_end);
}It looks very similar. A comment explaining why these 2 seemingly similar functions exist and when to which version would be good. While most of the code seems self-explanatory the places that are less obvious would greatly benefit from some documentation.
EDIT as per the comments:
I might be inclined to rewrite the pointer validity check functions as follows:
/** Check if the pointer is a valid location in the container */
bool is_valid_ptr() const
{
return (ptr >= container.mem_start
&& ptr < container.seq_end);
}
/** Check if the pointer is a valid location in the container or is the end pointer */
bool is_valid_ptr_or_end() const
{
return (is_valid_ptr() || ptr == container.seq_end);
}This change makes it a lot clearer to the reader what is going on here.
Code Snippets
void reallocate(unsigned size)
{
allocator_t al;
while (size == capacity())
{
size *= 2;
}pointer new_memstart = al.allocate(size), new_seqend;bool operator==(this_t const &rhs) const
{
return (ptr == rhs.ptr);
}
bool operator!=(this_t const &rhs) const
{
return (ptr != rhs.ptr);
}bool operator==(this_t const &rhs) const
{
return (ptr == rhs.ptr);
}
bool operator!=(this_t const &rhs) const
{
return !operator==(rhs);
}bool is_valid_ptr() const
{
return (ptr >= container.mem_start
&& ptr < container.seq_end);
}
bool is_valid_ptr2() const
{
return (ptr >= container.mem_start
&& ptr <= container.seq_end);
}Context
StackExchange Code Review Q#94211, answer score: 6
Revisions (0)
No revisions yet.