isovic / aligned_vector Goto Github PK

Hi isovic,
first of all, thank you for uploading this repo. Don't feel pressured to do anything it is too much of a hassle to fix the issue mentioned below. Also, I am not an expert in template container so don't take my word as some sort of criticism.

There are three problems I noticed that will critically affect the usability of this library.
I tested the library on Visual C++ 2019 and GCC 4.9.1. Both using C++11 standard.
I will report the issue on Visual C++ 2019 first and GCC later if I have more time.

1. Constructor for class 'is::aligned_vector' is declared 'explicit'

making the class not copy constructible on Visual C++ 2019

Consider the following code:

using data_t = uint16_t;
using aligned_data_t = is::aligned_vector<data_t, sizeof(uint64_t)>;
aligned_data_t  test_vector(4);
aligned_data_t  test_vector2 = test_vector; // does not compile

I think this is more of a design choice but not being able to assign vector to another one makes it hard to migrate from std::vector to is::aligned_vector.
It can easily be fixed by removing "explicit" keyword from

explicit aligned_vector(const aligned_vector& p)

2. the class member is not properly initialized when calling "aligned_vector(const aligned_vector& p) noexcept"

Consider the following custom class:

#ifndef RETURN_VECTOR_HPP_
#define RETURN_VECTOR_HPP_

#include <vector>
#include <iostream>

#define DEBUG_VERBOSE_
#include "aligned_vector.hpp"

using data_t = uint16_t;
using aligned_data_t = is::aligned_vector<data_t, sizeof(uint64_t)>;
using aligned_vector_t = std::vector<aligned_data_t>;

class return_vector
{
public:
    return_vector() :inner_vector(0){};
    aligned_vector_t return_vector_data(size_t size)
    {
        inner_vector = aligned_vector_t(size);
        for (size_t i = 0; i < inner_vector.size(); i++)
        {
            inner_vector[i] = aligned_data_t(i, data_t(i)); 
        }
        return inner_vector; // will deallocate temporary empty is::aligned_vector "data_" because "data_" is not NULL.
    }
private:
    aligned_vector_t inner_vector;
};

#endif

This will only cause problem in debug mode because VC++ set all the uninitialized value to some specific non-default value.
It can be fixed by changing

aligned_vector(const aligned_vector& p) noexcept

to

aligned_vector(const aligned_vector& p) noexcept : data_(NULL), raw_data_(NULL), size_(0), capacity_(0)

3. The vector is only valid when its data type is 1 byte long

This is the most critical issue here.
Consider the code in (2)

inner_vector[i] = aligned_data_t(i, data_t(i)); 

when copying the is::aligned_vector to another one
copy_from_ is called and it doesn't copy the data properly

void copy_from_(const aligned_vector& p) {
    if (&p == this) { return; }
    this->free_data_();
    this->reserve(p.capacity());
    this->size_ = p.size();
    memmove(data_, p.data(), p.size()); // should be memmove(data_, p.data(), sizeof(T)*p.size());
}

It could be fixed by changing the line to

memmove(data_, p.data(), sizeof(T)*p.size());

or

std::copy(p.data(), p.data() + p.size(), data_);

I will post some other issue regarding the test on GCC 4.9.1 if you are interested.
Again, thank you for this wonderful work. And I hope it won't bother you too much after I posted some issue after 4 years.