HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Vector implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationvectorstackoverflow

Problem

This is a C++ Vector that I've made for the pleasure of it. It was made on a whim and is not related any school assignment or anything alike this.

This Vector does not aim to be compliant with the standard library, and does not aim to replace std::vector. However, its goal is to be very efficient while being simple and easy to use. It also takes into account POD types to optimize for them, but this has to be specified by the Vector user.

  • Would you please take a look and let me know your impressions?



  • Does it do anything in a wrong way? Do you see any issues with it?



I am also wondering if I should use C++11 type support such as std::is_pod and remove the manual control of POD optimization.

I would also like "for each" to work with my vector, but I dislike the all-lower-case syntax of the required begin() and end() functions. Is there's a non-compiler specific way to specify which functions to use instead of the default begin() and end()?

Here is the header file and .cpp file which is included in the header. There are unit tests at ../CoreSpecimen/DataStruct/Vector.cpp but posting them here would surpass the character limits. I'm not looking for a review of the tests but feel free to do so if you would like.

Header:

```
/ Vector.hpp /

#pragma once
#include "../Types.hpp"
#include "../Memory/Memory.hpp"
#include "VectorAssert.hpp"

//Placement New
#ifdef CoreTargetWin32
inline void operator new(Core::UInt, void address){return address;}
inline void operator delete(void, void){}
#elif CoreTargetLinux
#ifdef CoreTarget64Bits
inline void operator new(long unsigned int, void address){return address;}
#else
inline void operator new(unsigned int, void address){return address;}
#endif
inline void operator delete(void, void){}
#endif

namespace Core
{
namespace DataStruct
{
template class Vector
{
public:
enum CtorModeEnum{Always, Once, Pod};

Solution

When you include source from a header file its best to name the file differently:

#include "Vector.cpp"

// Normally when including template source name the file tpp

#include "Vector.tpp"


Your reverse iterators are not going to work as epected:

template typename Vector::ConstElement* Vector::RBegin() const
{
    return _last - 1;
}

template typename Vector::ConstElement* Vector::REnd() const
{
    return _origin - 1;
}


When you increment the reverse iterator it should head in the reverse direction to the normal iterator (so it needs some wrapper to do that). Luckily there is such a wrapper in the standard library

At first glance a lot of your manual movement/copying of stuff around are covered by standard algorithms. Though the standard algorithms don't use run time checks they do it at compile time so would be much more efficient to use the standard algorithms.

if(_ctorMode != CtorModeEnum::Always)
        Memory::Copy(_origin, newOrigin, sizeof(Element) * Length());
    else
    {
        MoveRange(newOrigin, _origin, _last);
    }


I would have written this as:

// The compiler decides if it is movable at compile time.
    // If not it will copy it. If it is a POD type it will just use
    // a memmov() otherwise it has to invoke the constructor appropriately.
    std::move(_origin, _last, newOrigin);

Code Snippets

#include "Vector.cpp"

// Normally when including template source name the file tpp

#include "Vector.tpp"
template<class T> typename Vector<T>::ConstElement* Vector<T>::RBegin() const
{
    return _last - 1;
}

template<class T> typename Vector<T>::ConstElement* Vector<T>::REnd() const
{
    return _origin - 1;
}
if(_ctorMode != CtorModeEnum::Always)
        Memory::Copy(_origin, newOrigin, sizeof(Element) * Length());
    else
    {
        MoveRange(newOrigin, _origin, _last);
    }
// The compiler decides if it is movable at compile time.
    // If not it will copy it. If it is a POD type it will just use
    // a memmov() otherwise it has to invoke the constructor appropriately.
    std::move(_origin, _last, newOrigin);

Context

StackExchange Code Review Q#74521, answer score: 2

Revisions (0)

No revisions yet.