patterncppMinor
c++ std::array implementation
Viewed 0 times
stdimplementationarray
Problem
Just after a review of my implementation of std::array. I created this to find the sweet spot between speed and nice code. I would really appreciate any guidance towards improving.
Note: I use parts of my meta-programming library you can find: https://github.com/sigma-blight/sigma_api [sigma_api/sigma/meta/].
``
* from a type Vp_ to a type Tp_
*/
template
using EnableCMem = meta::BoolConstant::value &&
std::is_pod::value
>;
/* Store size of data array in type
* for zero overhead
*/
using DataSize = meta::SizeConstant;
/* Raw Data
*/
Tp_ _data[n_];
public:
//============================================
// Initialisation
//============================================
/* Maintain default initialisation
* from compilier
*
* default:
* ~ Default Ctor
* ~ Copy Ctor
* ~ Move Ctor
* ~ Destructor
* ~ Copy Assignment Operator
* ~ Move Assignment Operator
*/
Array(void) = default;
Array(const Array &) = default;
Array(Array &&) = default;
~Array(void) = default;
Array & operator = (const Array &) = default;
Array & operator = (Array &&) = default;
/* Values Constructor
*
* move a variadic list of values into the raw data
*
* enabled when exactly n_ many values are provided
*
* @tparam Tps_ -> parameter pack of arguments to move assign
*
* ASSERT: Every Tps_ is move assignable
*/
template == n_
>>
Array(Tps_ && ... values)
: _data{ std::move(values) ... }
{}
//============================================
// **
Note: I use parts of my meta-programming library you can find: https://github.com/sigma-blight/sigma_api [sigma_api/sigma/meta/].
``
/* Static Size Container Array
*
* Wrapper class to a c-array of Tp_[n_]
*
* @tparam Tp_ -> element type
* @tparam n_ -> size of array
*/
template
class Array
{
/* EnableCMem
* check for the ability to
* use memcpy and memmove`* from a type Vp_ to a type Tp_
*/
template
using EnableCMem = meta::BoolConstant::value &&
std::is_pod::value
>;
/* Store size of data array in type
* for zero overhead
*/
using DataSize = meta::SizeConstant;
/* Raw Data
*/
Tp_ _data[n_];
public:
//============================================
// Initialisation
//============================================
/* Maintain default initialisation
* from compilier
*
* default:
* ~ Default Ctor
* ~ Copy Ctor
* ~ Move Ctor
* ~ Destructor
* ~ Copy Assignment Operator
* ~ Move Assignment Operator
*/
Array(void) = default;
Array(const Array &) = default;
Array(Array &&) = default;
~Array(void) = default;
Array & operator = (const Array &) = default;
Array & operator = (Array &&) = default;
/* Values Constructor
*
* move a variadic list of values into the raw data
*
* enabled when exactly n_ many values are provided
*
* @tparam Tps_ -> parameter pack of arguments to move assign
*
* ASSERT: Every Tps_ is move assignable
*/
template == n_
>>
Array(Tps_ && ... values)
: _data{ std::move(values) ... }
{}
//============================================
// **
Solution
You did not provide the implementation of your constructors or assignment operators so its hard to tell if they are good or not.
Also nice to add easier access to const iterators:
Otherwise you need to cast the object to const to get accesses to the const version of
I don't see the need for this:
Yes I see what you are doing. But you other version already does this under the covers.
Because
Same comment applied to the
Using the assignment operators to implement swap. Interesting.
Ok it looks like it could work but this is probably not the most efficient way to swap two arrays.
First you make a copy (using move construction). Which means moving all the elements out of
So you are basically doing three sets of moves, where each move is probably implemented as a swap operation. It would be easier to just run through the arrays and swap the elements in the arrays.
The operators section. I see no need for any of these. That is why we have algorithms.
Also nice to add easier access to const iterators:
const Tp_ * cbegin(void) const { return Array::_data; }
const Tp_ * cend(void) const { return Array::_data + n_; }
// ^ add the cOtherwise you need to cast the object to const to get accesses to the const version of
begin() and end().I don't see the need for this:
template
meta::EnableIf_t::value>
copy(const Array & array)
{
memcpy(Array::_data, array.begin(), DataSize::value);
}Yes I see what you are doing. But you other version already does this under the covers.
template
meta::EnableIf_t::value>
copy(const Array & array)
{
static_assert(meta::CopyAssignable_v,
"Array::copy(const Array &) ~ Invalid Vp_, requires copy assignability from const Vp_ to Tp_ \n");
std::copy(array.begin(), array.end(), Array::begin());
}Because
std::copy() already has the best version built in. There is no need to optimize something that is already optimized. If it is not optimized by your standard library, then they have already measured it and determined it is not worth the effort. Same comment applied to the
move().Using the assignment operators to implement swap. Interesting.
Ok it looks like it could work but this is probably not the most efficient way to swap two arrays.
template
void swap(Array & array)
{
auto array_temp = std::move(array);
array = std::move(*this);
*this = std::move(array_temp);
}First you make a copy (using move construction). Which means moving all the elements out of
array into array_temp. Next you move assign. This means moving all the elements from this into array. Then your last step is to move all the elements from temp array into `this.So you are basically doing three sets of moves, where each move is probably implemented as a swap operation. It would be easier to just run through the arrays and swap the elements in the arrays.
template
void swap(Array & array)
{
for(Size i = 0; i operator[](i), array[i]);
}
}The operators section. I see no need for any of these. That is why we have algorithms.
Code Snippets
const Tp_ * cbegin(void) const { return Array::_data; }
const Tp_ * cend(void) const { return Array::_data + n_; }
// ^ add the ctemplate<typename Vp_>
meta::EnableIf_t<EnableCMem<const Vp_>::value>
copy(const Array<Vp_, n_> & array)
{
memcpy(Array::_data, array.begin(), DataSize::value);
}template<typename Vp_>
meta::EnableIf_t<!EnableCMem<const Vp_>::value>
copy(const Array<Vp_, n_> & array)
{
static_assert(meta::CopyAssignable_v<Tp_, const Vp_>,
"Array<Tp_, n_>::copy(const Array<Vp_, n_> &) ~ Invalid Vp_, requires copy assignability from const Vp_ to Tp_ \n");
std::copy(array.begin(), array.end(), Array::begin());
}template<typename Vp_>
void swap(Array<Vp_, n_> & array)
{
auto array_temp = std::move(array);
array = std::move(*this);
*this = std::move(array_temp);
}template<typename Vp_>
void swap(Array<Vp_, n_> & array)
{
for(Size i = 0; i < n; ++i) {
using std::swap;
swap(this->operator[](i), array[i]);
}
}Context
StackExchange Code Review Q#157273, answer score: 5
Revisions (0)
No revisions yet.