patterncppMinor
C++ Vector The basics
Viewed 0 times
basicsthevector
Problem
Following on from my two previous posts.
I have written a detailed blog about how to write a minimal vector like class. This set of articles has been inspired by multiple posts here on http://codereview.stackexchange.com (See Sources).
The final result is below.
But now it is my turn for some review to make sure I did not screw up too much. :-)
Head:
Types:
Constructors:
```
public:
Vector(int capacity = 10)
: capacity(capacity)
, length(0)
, buffer(static_cast(::operator new(sizeof(T) * capacity)))
{}
template
Vector(I begin, I end)
: capacity(std::distance(begin, end))
, length(0)
, buffer(static_cast(::operator new(sizeof(T) * capacity)))
{
for(auto loop = begin;loop != end; ++loop)
{
- An alternative vector
- An Alternative Vector (Copy Assignment Operator)
I have written a detailed blog about how to write a minimal vector like class. This set of articles has been inspired by multiple posts here on http://codereview.stackexchange.com (See Sources).
- Index
- Resource Management: Allocation
- Resource Management: Copy and Swap
- Resource Management: Resize
- Resource Management: Simple Optimization
- The Other Stuff
The final result is below.
But now it is my turn for some review to make sure I did not screw up too much. :-)
Head:
#ifndef THORSANVIL_CONTAINER_VECTOR
#define THORSANVIL_CONTAINER_VECTOR
#include
#include
#include
#include
#include
#include
namespace ThorsAnvil
{
namespace Container
{Types:
template
class Vector
{
public:
using value_type = T;
using reference = T&;
using const_reference = T const&;
using pointer = T*;
using const_pointer = T const*;
using iterator = T*;
using const_iterator = T const*;
using difference_type = std::ptrdiff_t;
using size_type = std::size_t;
private:
size_type capacity;
size_type length;
T* buffer;
struct Deleter
{
void operator()(T* buffer) const
{
::operator delete(buffer);
}
};Constructors:
```
public:
Vector(int capacity = 10)
: capacity(capacity)
, length(0)
, buffer(static_cast(::operator new(sizeof(T) * capacity)))
{}
template
Vector(I begin, I end)
: capacity(std::distance(begin, end))
, length(0)
, buffer(static_cast(::operator new(sizeof(T) * capacity)))
{
for(auto loop = begin;loop != end; ++loop)
{
Solution
I've only had a chance to glance at things so far, but this almost jumped out at me:
This seems rather convoluted, at least to me. I'd prefer:
I believe this shows the intent much more directly, and (thanks to short-circuit evaluation) still avoids evaluating
Some of the names also seem somewhat misleading. For example, I'd rename
Oh, looking at the multiplier you've used: 1.62 is a poor choice. The point of using the golden mean is that it's the largest multiplier you can use that guarantees that (if they're contiguous) the discarded pieces will eventually add together to be large enough to fulfill the next larger request size. Note that wording carefully though: the golden mean is the largest multiplier you can use and still get this result. By rounding up from the golden mean (1.618...) to 1.62, you've basically assured this can't happen.
To get the desired behavior, you need to use a multiplier that's less than or equal to the golden mean. In most cases, the memory manager will add a little overhead to the block size you use, so if you use exactly the golden mean, it'll never happen either. I'd recommend a multiplier no greater than 1.6. In many cases, it's easiest to use 1.5, which makes integer math easy, and still gets roughly the same effects (and a fair amount of empirical testing indicates works well in general).
bool operator==(Vector const& rhs) const
{
return (size() == rhs.size())
? std::equal(begin(), end(), rhs.begin())
: false;
}This seems rather convoluted, at least to me. I'd prefer:
bool operator==(Vector const& rhs) const
{
return (size() == rhs.size()) && std::equal(begin(), end(), rhs.begin());
}I believe this shows the intent much more directly, and (thanks to short-circuit evaluation) still avoids evaluating
std::equal if the sizes aren't equal.Some of the names also seem somewhat misleading. For example, I'd rename
resizeIfRequire to something like reallocIfRequired, since it can only affect the capacity, not the size.Oh, looking at the multiplier you've used: 1.62 is a poor choice. The point of using the golden mean is that it's the largest multiplier you can use that guarantees that (if they're contiguous) the discarded pieces will eventually add together to be large enough to fulfill the next larger request size. Note that wording carefully though: the golden mean is the largest multiplier you can use and still get this result. By rounding up from the golden mean (1.618...) to 1.62, you've basically assured this can't happen.
To get the desired behavior, you need to use a multiplier that's less than or equal to the golden mean. In most cases, the memory manager will add a little overhead to the block size you use, so if you use exactly the golden mean, it'll never happen either. I'd recommend a multiplier no greater than 1.6. In many cases, it's easiest to use 1.5, which makes integer math easy, and still gets roughly the same effects (and a fair amount of empirical testing indicates works well in general).
Code Snippets
bool operator==(Vector const& rhs) const
{
return (size() == rhs.size())
? std::equal(begin(), end(), rhs.begin())
: false;
}bool operator==(Vector const& rhs) const
{
return (size() == rhs.size()) && std::equal(begin(), end(), rhs.begin());
}Context
StackExchange Code Review Q#123402, answer score: 6
Revisions (0)
No revisions yet.