patterncppMinor
Simplified implementation of std::vector in C++
Viewed 0 times
stdsimplifiedimplementationvector
Problem
I implemented a simplified version of a C++
Because of those 4 areas I'd like to focus on practicing, I only had extra add and remove method in the class.
Please focus on those 4 areas I mentioned for reviewing my code, and any other important style choice errors or any other problems that I missed.
```
using namespace std;
template
class ArrayList
{
public:
ArrayList();
ArrayList(int size);
ArrayList(const ArrayList& other);
ArrayList(ArrayList&& other);
~ArrayList();
ArrayList& operator= (const ArrayList& other);
ArrayList& operator= (ArrayList&& other);
void add(const T& item);
void add(T&& item);
void remove(int index);
friend void swap(ArrayList& A, ArrayList& B)
{
using std::swap;
swap(A.actualSize, B.actualSize);
swap(A.allocatedSize, B.allocatedSize);
swap(A.arr, B.arr);
}
private:
T* arr;
size_t allocatedSize;
size_t actualSize;
void resize();
void addInternal(const T& item);
void addInternalMove(T&& item);
};
template
ArrayList::ArrayList()
{
arr = static_cast(::operator new(sizeof(T)*100));
actualSize = 0;
allocatedSize = 100;
}
template
ArrayList::ArrayList(int size)
{
if(size (::operator new(sizeof(T)*size));
actualSize = 0;
allo
std::vector/ArrayList learning from this link. From it, I tried to write an ArrayList class rather than a Vector. In my ArrayList implementation, I didn't add iterators, operator[], emplace and a lot of other stuff because my main focus is to practice the following aspects:- My
ArrayListclass guarantees strong exception safety, using the copy and swap idiom
- The container still works if
Tpassed in is not default-constructible
- Correctness of the implementation of the Rule of Five
- General correctness and efficiency (i.e: No memory leaks, dangling pointers, etc.)
Because of those 4 areas I'd like to focus on practicing, I only had extra add and remove method in the class.
Please focus on those 4 areas I mentioned for reviewing my code, and any other important style choice errors or any other problems that I missed.
```
using namespace std;
template
class ArrayList
{
public:
ArrayList();
ArrayList(int size);
ArrayList(const ArrayList& other);
ArrayList(ArrayList&& other);
~ArrayList();
ArrayList& operator= (const ArrayList& other);
ArrayList& operator= (ArrayList&& other);
void add(const T& item);
void add(T&& item);
void remove(int index);
friend void swap(ArrayList& A, ArrayList& B)
{
using std::swap;
swap(A.actualSize, B.actualSize);
swap(A.allocatedSize, B.allocatedSize);
swap(A.arr, B.arr);
}
private:
T* arr;
size_t allocatedSize;
size_t actualSize;
void resize();
void addInternal(const T& item);
void addInternalMove(T&& item);
};
template
ArrayList::ArrayList()
{
arr = static_cast(::operator new(sizeof(T)*100));
actualSize = 0;
allocatedSize = 100;
}
template
ArrayList::ArrayList(int size)
{
if(size (::operator new(sizeof(T)*size));
actualSize = 0;
allo
Solution
Here's the original code with comments inserted:
Change this constructor to use size_t:
Should be:
Change parameter of
Should be:
Eliminate redundant
Include named constant for
Avoid repetitive code, implement
Avoid
Should be:
Syntax error:
setting
will increment
Should be:
Really, I'd delegate to
Syntax error: method should return a value.
the exit to the method and destructor won't work.
Could be:
But this is more efficient:
Add a line break after
Should be:
Add
If you hit an exception in the
Should be:
no-op.
Should be:
If
compiler will not be able to implicitly fall back to the version of
just
This code has a bunch of flaws.
exception safe. You call the detructor on
moving things "up" rather than "down". It's inefficient to loop from
I'd rewrite:
As:
```
template
void ArrayList::remove(size_t index)
{
// Allow array to shrink. It's possible this is not desired.
size_t newSize = allocatedSize / 2;
if (newSize tmp(newSize);
if(index + 1>actualSize)
throw;
for(size_t i=0; i<index; i
Change this constructor to use size_t:
ArrayList(int size);Should be:
ArrayList(size_t size);Change parameter of
remove to be a size_t.void remove(int index);Should be:
void remove(size_t index);Eliminate redundant
usingfriend void swap(ArrayList& A, ArrayList& B)
{
using std::swap;
// --snip--
}Include named constant for
100:private:
// --snip--
const size_t defaultSize = 100;
};Avoid repetitive code, implement
ArrayList::ArrayList() in terms ofArrayList::ArrayList(size_t).template
ArrayList::ArrayList()
: ArrayList::ArrayList(ArrayList::defaultSize)
{
}Avoid
::operator new(0) and allocatedSize == 0template
ArrayList::ArrayList(int size)
{
if(size < 0)
throw;
// --snip--
}Should be:
template
ArrayList::ArrayList(size_t size)
{
if(size < 1)
throw;
// --snip--
}Syntax error:
size is undefined. You mean to use other.actualSize whensetting
actualSize, but really, you need to use 0 since add/addInternalwill increment
actualSize. You can safely use addInternal sinceallocatedSize is set appropriately large.template
ArrayList::ArrayList(const ArrayList& other)
{
arr = static_cast(::operator new(sizeof(T)*size));
allocatedSize = other.allocatedSize;
actualSize = actualSize;
for(size_t i = 0; i<other.actualSize; i++)
add(other.arr[i]);
}Should be:
template
ArrayList::ArrayList(const ArrayList& other)
{
arr = static_cast(::operator new(sizeof(T)*other.allocatedSize));
// --snip--
actualSize = 0;
// --snip--
addInternal(other.arr[i]);
}Really, I'd delegate to
ArrayList(size_t)template
ArrayList::ArrayList(const ArrayList& other)
: ArrayList::ArrayList(other.allocatedSize)
{
for(size_t i = 0; i<other.actualSize; i++)
addInternal(other.arr[i]);
}Syntax error: method should return a value.
other will be uninitialized atthe exit to the method and destructor won't work.
template
ArrayList::ArrayList(ArrayList&& other)
{
swap(*this, other);
}Could be:
template
ArrayList::ArrayList(ArrayList&& other)
: ArrayList::ArrayList(1)
{
swap(*this, other);
return *this;
}But this is more efficient:
template
ArrayList::ArrayList(ArrayList&& other)
{
actualSize = other.actualSize;
allocatedSize = other.allocatedSize;
arr = other.arr;
// set other.arr to a value that doesn't incur the
// cost of a new, but can be deleted without heap
// corruption.
other.arr = nullptr;
// avoid calling destuctor on elements of the array
// in destructor call on other.
other.actualSize = 0;
return *this;
}Add a line break after
for loop.template
ArrayList::~ArrayList()
{
for(size_t i = 0; i<actualSize; i++)
{
arr[i].~T();
}::operator delete(arr);
}Should be:
template
ArrayList::~ArrayList()
{
for(size_t i = 0; i<actualSize; i++)
arr[i].~T();
::operator delete(arr);
}Add
return *this;:template
ArrayList& ArrayList::operator =(ArrayList&& other)
{
swap(*this, other);
return *this;
}If you hit an exception in the
for loop, this is corrupt since you mutateallocatedSize.template
void ArrayList::resize()
{
allocatedSize *= 2;
ArrayList tmp(allocatedSize);
// --snip--
}Should be:
template
void ArrayList::resize()
{
ArrayList tmp(allocatedSize * 2);
// --snip--
}move is redundant; item is already a T&& so the cast done by move is ano-op.
template
void ArrayList::add(T&& item)
{
// --snip--
addInternalMove(move(item));
}Should be:
template
void ArrayList::add(T&& item)
{
// --snip--
addInternalMove(item);
}If
T does not define T::T(T&& other), addInternalMove won't work and thecompiler will not be able to implicitly fall back to the version of
addInternal with copy semantics. Consider renaming addInternalMove tojust
addInternal.This code has a bunch of flaws.
actualSize is not decremented. It's notexception safe. You call the detructor on
arr[i], but then assign it. You'removing things "up" rather than "down". It's inefficient to loop from
0 toindex doing a no-op. actualSize-1 has interger underflow if actualSize is 0.I'd rewrite:
template
void ArrayList::remove(int index)
{
if(indexactualSize - 1)
throw;
for(size_t i=0; i i)
arr[i+1] = arr[i];
}
}As:
```
template
void ArrayList::remove(size_t index)
{
// Allow array to shrink. It's possible this is not desired.
size_t newSize = allocatedSize / 2;
if (newSize tmp(newSize);
if(index + 1>actualSize)
throw;
for(size_t i=0; i<index; i
Code Snippets
ArrayList(int size);ArrayList(size_t size);void remove(int index);void remove(size_t index);friend void swap(ArrayList& A, ArrayList& B)
{
using std::swap;
// --snip--
}Context
StackExchange Code Review Q#136143, answer score: 4
Revisions (0)
No revisions yet.