tags:

views:

811

answers:

11

Will the array be deallocated and if so, what is a workaround?

double * GetArrayFromVector( std::map<std::string, double> m, char ** names, int count )
{ 
    if(!names) return 0;

    std::vector<double> vec(m.size());
    for (int i=0; i<count; ++i)
    { 
       if(!names[i]) return 0;
       std::map<std::string, double>::iterator iter=m.find(name[i]);
       if(iter!=m.end())
          vec.push_back(iter->second);
       else
         return 0;   
    }

    return &vec[0]; 
}

Thanks a lot

+3  A: 

Yes the array will be deallocated.

Change the function to:

double * GetArrayFromVector( std::map<std::string, double> m, vector<double> &vec, char ** names, int count )
{ 
     vec.clear();
     vec.reserve(m.size());

     for (int i=0; i<count; ++i)
     { 
         if(!names[i]) return 0;

         std::map<std::string, double>::iterator iter=m.find(name[i]);
         if(iter!=m.end())
            vec.push_back(iter->second);
         else
           return 0;   
     }

    return &vec[0]; 
}

Or else use boost::shared_array (also, look at boost::scoped_array)

boost::shared_array<double> GetArrayFromVector( std::map<std::string, double> m, char ** names, int count )
{ 
     boost::shared_array<double> vec(new double[m.size()]);

     for (int i=0; i<count; ++i)
     { 
         if(!names[i]) return boost::shared_array<double>();

         std::map<std::string, double>::iterator iter=m.find(name[i]);
         if(iter!=m.end())
            vec[i] = iter->second;
         else
           return boost::shared_array<double>();   
     }

    return vec; 
}
Eclipse
I am a firm believer in the shared_array idea. I would go even further, using scoped_array and forcing a swap.
Benoît
scoped_array is a little harder to use, but is probably a better fit in this situation (although that may depend on how the return value is used)
Eclipse
Guys, this is an overkill for this problem....thanks for the idea
i think scoped_array with using swap would be awkward here. it would "hack" move semantics into it while it does not have them. swap is just for swapping, not for transfering ownership i think. std::unique_ptr would be ideal though i think. or shared_ptr since boost does not have a public move ptr.
Johannes Schaub - litb
A: 

You can pass it by reference or new/delete it, but as posted your vector will be destructed after the function returns.

Ron Warholic
A: 

Yes, the vector (and the data store it holds) will be deallocated when the function ends.

Why are you creating a vector? If you want an array, just create & fill one of those..

double * GetArrayFromVector( std::map<std::string, double> m, char * names[], int count )
{ 
    if(!names) return 0;

    double* vec = new double[m.size()];
    int j = 0;
    for (int i=0; i<count; ++i)
    { 
       if(!names[i]) return 0;
       std::map<std::string, double>::iterator iter=m.find(name[i]);
       if(iter!=m.end())
          vec[j++] =iter->second;
       else
         return 0;   
    }

    return vec;
}
James Curran
you do NOT need j in there.
You're right. I wasn't paying attention to the code, and thought it skipped over null names instead of aborting the function.
James Curran
+6  A: 

Yes -- it's deallocated as soon as your return from the function, because vec is declared on the stack. The std::vector destructor takes care of freeing the memory. Since you're returning the address of a deallocated array, you're going to start messing around with deallocated memory, which is a big no-no. At best, you'll crash immediately. At worst, you'll silently succeed with a big gaping security hole.

There are two ways to fix this: (1) return the entire vector by-value, which makes a copy of the entire vector, or (2) return the vector via a reference parameter.

Solution 1:

std::vector<double> GetArrayFromVector(...)
{
    ...
    return vec;  // make copy of entire vec, probably not a good idea
}

Solution 2:

void GetArrayFromVector(..., std::vector<double> & vec)
{
    // compute result, store it in vec
}
Adam Rosenfield
+1. Great straightforward answer.
Kristo
A: 

Yes the vector will be deallocated when the function exits (and the 'vec' variable goes out of scope). The pointer you return will therefore be invalid.

An alternative is to allocate the array on the heap (using the 'new' operator) and return that pointer instead, but it will be the caller's responsibility to 'delete' the pointer, which is usually frown upon.

A better alternative is to return a shared_ptr to your array.

Martin Cote
shared_ptr will not work with arrays, since they call delete, not delete[] upon destruction.
Eclipse
+2  A: 

vec is a local variable. Its scope is limited to the GetArrayFromVector() function only. Never return the address of a local variable. Either return the array by value:

std::vector<double> GetArrayFromVector( std::map<std::string, double> m,
                         char ** names, int count )

or, pass a reference to the vector as an output parameter:

void GetArrayFromVector( std::map<std::string, double> m,
                         char ** names, int count, 
                         std::vector<double>& vec)

or, pass an output iterator:

void GetArrayFromVector( std::map<std::string, double> m,
                         char ** names, int count, 
                         std::vector<double>::iterator vecIter)

The last two will require some careful implementation of the function definition and calling though.

Additionally, if you are game for a bit more adventure try this:

// you'd need to change the value to use when an element is not
// found in the map to something that suits your needs
double pred(std::map<char*, double> haystick, char* const needle) {
    std::map<char*, double>::iterator i = haystick.find(needle);
    return i != haystick.end() ? i->second : 0; 
}

int main(int argc, char* argv[])
{

   std::map<char *, double> m;
   std::vector<char *> names;
   std::vector<double> dv;

   m[ "Sasha" ] = 729.0;
   m[ "josh" ] = 8154.0;

   names.push_back("Sasha");
   names.push_back("JonSkeet");
   names.push_back("josh");

   // transform is part of STL's <algorithm> header
   // it takes a container (actually a range -- [begin(), end()) 
   //                  note it is a half-open range -----------^
   // as is customary for all STL algorithms, applies the function
   // or functor specified as the last parameter to each element of
   // the sequence and writes the result back to another container
   // specified via the output iterator -- the third argument
   //
   // since I have not reserved enough elements for the vector dv
   // i cannot blindly use it -- i need a back_inserter to coax
   // transform to push_back() instead of do an insert operation
   // of course, for vectors, this is costly since reallocations 
   // may happen, but let's leave the performance aside for a while!
   //
   // ok, so what about the last parameter, you ask? it has to be an
   // unary_operation. well, mostly so. but what is it that we want?
   // we want to take an iterator from the original char* (string) 
   // array and see if there's an entry in the map. if there is one
   // we retrieve the associated double value and put it in dv; else,
   // we set a default value of 0 -- change it to whatever pleases you
   // maybe a std::numeric_limit<double> if it works for you.
   // 
   // you can create a functor inheriting std::unary_function and pass
   // it on. that's the easy way out. but what if you already have a
   // comparator, a C-style find() function? will it work? yes, it will.
   // but we have to wrap it using the function adaptor std::ptr_fun
   // to make the compiler happy (after all it wants a unary_function, right?)
   // 
   // this simple scheme of things works very well, save for a last little
   // glitch. the comparator actually takes two parameters -- a what to search
   // and a where to search. and guess what -- the where to search is always 
   // fixed. so that gives us a good oppertunity to fix the first parameter to
   // our map<char*, double> which is exactly what std::bind1st() does. 
   // surprisingly, now that you've fixed one function, you no longer have a
   // binary function (one taking two arguments) but an unary one -- which is
   // just what you'd pass to transform. voila!
   std::transform(names.begin(), names.end(), std::back_inserter(dv), 
       std::bind1st(std::ptr_fun(pred), m));

   std::copy(dv.begin(), dv.end(), 
       std::ostream_iterator<double>(std::cout, "\n"));

    return 0;
}

Some interesting links:

Also check out Boost. They have done some magic with bind()!

dirkgently
Very good solution: a) I need array of doubles as an output b) please explain this line: std::transform(names.begin(), names.end(), std::back_inserter(dv), std::bind1st(std::ptr_fun(pred), m));
added comments in code -- being plain lazy ;)
dirkgently
Great explanation
A: 

Since you know count up front, there's no benefit to using an stl vector. You could simply do this:

double* GetArrayFromVector(std::map<char*, double> m, char** names, int count)
{
    double* result = new double[count];
    for (int i = 0; i < count; ++i)
    { 
        if(!names[i])
        {
            delete[] result;
            return 0;
        }
        map<std::string, double>::iterator iter = m.find(name[i]);
        if(iter != m.end())
        {
            result[i] = iter->second;
        }
        else
        {
            delete[] result;
            return 0;
        }
    }
    return result;
}

Note that you are passing ownership of the allocated array to the caller. Personally, I'd try to write the code in a way that observes the RAII principle; either pass in a vector to be populated, or use a managed/shared pointer, etc. (both of these options have been suggested in other answers).

zweiterlinde
However, you just removed any hope of having exception-safe code. Current C++ wisdom avoids raw pointers wherever possible.
Eclipse
easily error prone, as I can miss a delete. Otherwise, it works. thx
Agree with both comments. Personally, I prefer passing in a vector to populate, or returning a managed pointer of some kind.
zweiterlinde
yeah, you should use std::vector here. there was a proposal to add std::dynarray for exactly this purpose (where you want to have a dynamic, fixed array) similar to variable length arrays in C but wrapped up a class. i dunno whether it was accepted. otherwise, i would use shared_array too.
Johannes Schaub - litb
+5  A: 

Divide your function on two. Make your functions make just one action:
1. fill vector from map.
2. create array from vector.
Don't forget to pass map by const reference.

Main note: caller of the GetArrayFromVector is responsible for memory deallocation.

void FillVector( const std::map<std::string, double>& m, 
                  std::vector< double >& v, 
                  char ** names, 
                  int count )
 {
       .......
 }

 double* createArray( const std::vector< double >& v )
 {
     double* result = new double [v.size()];

     memcpy( result, &v.front(), v.size() * sizeof( double ) );

     return result; 
 }  

 // and finally your function

 double* GetArrayFromVector( const std::map<std::string, double>& m,  
                             char ** names, 
                             int count )
 {
      std::vector< double > v;
      FillVector( m, v, names, count );

      return CreateArray( v );
 }
Mykola Golubyev
A good suggestion. However, I, at the end, need an array of double. Hence, no vector is needed. you can edit your response to reflect that
@Sasha: done. I didn't test this though.
Mykola Golubyev
A: 

There is a concept call move constructors that would allow you to transfer the ownership of a resource held by a (stack-based) object to a new object. I have heard that STLport has a move_source template to accomplish this

This will be coming to C++0x.

In this case, you would be returning std::vector<double> instead of double*.

chrish
A: 
Darius Kucinskas
A: 

Kinda surprised no one has mentioned vector::swap. Have the caller pass in a reference to a vector which will have its contents replaced by the function:

void GetArrayFromVector( std::vector<double>& output, ... )
{ 
    std::vector<double> vec(m.size());

    // construct vec here...

    output.swap(vec); 
}

BTW: "GetArrayFromVector" and you pass in a map?

Alastair