current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

Thought I share this piece of code to the world. But be aware, I am not sure if this piece of code is safe and efficient. Feel free to improve it or give some feedback and suggestions.

#pragma region DOCUMENTATION ON: STD_COPY_VECTOR_TO_ARRAY
//////////////////////////////////////////////////////////////////////////////
// MACRO: STD_COPY_VECTOR_TO_ARRAY( Vector, Position, Length, Array )       //
//                                                                          //
// DESCRIPTION:                                                             //
// A defined macro that is created to copy selected values of std::vector   //
// an array. This defined macro takes 4 arguments.                          //
//                                                                          //
// ARGUMENTS:                                                               //
// Vector - The std::vector that contains the values that will be copied.   //
// Position - Position of the first value from Vector to be copied.         //
// Note: The first value of Position is marked by the value of 0 and not 1. //
// Length - Amount of values to be copied from Vector.                      //
// Array - The Array that the values will be copied to.                     //
//                                                                          //
// TIPS:                                                                    //
// #1 - To copy all values within Vector, set the value of Position to 0    //
// and set the value of Length to the size of Vector (std::vector::size).   //
//////////////////////////////////////////////////////////////////////////////
#pragma endregion
#define STD_COPY_VECTOR_TO_ARRAY( Vector, Position, Length, Array )         \
                                  std::copy( Vector.begin( ) + Position,    \
                                  Vector.begin( ) + Position + Length ,     \
                                  Array )
share|improve this question
    
This site is not for sharing code, it's for requesting review. And since that doesn't seem to be the primary reason you posted, I think this “question” is off topic. –  svick Feb 26 '13 at 1:40
    
@svick: It does not matter what his original intention was. As long as somebody else can learn from there mistakes by reading the below discussion. –  Loki Astari Feb 26 '13 at 5:34
add comment

2 Answers

up vote 4 down vote accepted

Basically, there are very few places in C++ where you want to use Macros. A lot of old C code used function-like macros because of performance reasons - calling a function requires creating a new stack frame. They were also used to get away from C's type system. With C++, inline functions and templates remove the need for this 99% of the time.

template <typename T>
inline void copy_vector_to_array(const std::vector<T>& vec, 
                                 std::size_t position, 
                                 std::size_t length, 
                                 T* array)
{
    std::copy(vec.begin() + position, vec.begin() + position + length, array);
}

In C++11, we can do better still - an erroneous length passed in above will cause problems, and will be hard to detect:

#include <vector>
#include <array>
#include <iterator>
#include <cassert>

template <typename T, std::size_t N>
inline void copy_vector_to_array(const std::vector<T>& vec, 
                                 std::size_t position, 
                                 std::size_t length, 
                                 std::array<T, N>& array)
{
    assert(length <= N);
    std::copy(std::begin(vec) + position, std::begin(vec) + position + length, 
              std::begin(array));
}

This will be just as efficient as the macro version, however, we also gain type safety, as well as (easy) bounds checking.

We can also improve on it. So that it handles other container types:

template <typename C, std::size_t N>
inline void copy_container_to_array(const C& container, 
                                 std::size_t position, 
                                 std::size_t length, 
                                 std::array<typename C::value_type, N>& array)
{
    assert(length <= N);
    std::copy(std::next(std::begin(vec), position), std::next(std::begin(vec), position + length), 
              std::begin(array));
}
share|improve this answer
    
Nicely done Yuushi! –  CLearner Feb 26 '13 at 1:34
    
Isn't the inline modifier ignored by most modern compilers? –  svick Feb 26 '13 at 1:41
    
@svick It's a suggestion that the compiler is free to ignore based on whatever code analysis it does, and likely at O3 will be somewhat superfluous. –  Yuushi Feb 26 '13 at 1:52
    
We can improve on that by making it handle other container types :-) –  Loki Astari Feb 26 '13 at 5:25
    
@LokiAstari True, best is to just work with iterators directly, which doesn't constrain you to container types at all. –  Yuushi Feb 26 '13 at 5:29
show 1 more comment
up vote 2 down vote

Since you're specifying the number of items rather than the start and end items, you might as well use the algorithm defined specifically for that purpose:

std::copy_n(std::next(std::begin(Vector), Position), Length, std::begin(Array));

Given the triviality of the code and its applicability to situations other than copying from a vector to an array, creating either a macro or an inline function for it seems a bit silly to me though -- more likely a loss than an improvement.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.