CODEWITHSUNDEEP
c++pointersstlvector
zzzbbx
zzzbbx

Reputation: 10131

newbie question about pointer to stl

I have written this function

vector<long int>* randIntSequence(long int n) {
    vector<long int> *buffer = new vector<long int>(n, 0);
    for(long int i = 0; i < n; i++)
        buffer->at(i);

    long int j; MTRand myrand;
    for(long int i = buffer->size() - 1; i >= 1; i--) {
        j = myrand.randInt(i);
        swap(buffer[i], buffer[j]);
    }
    return buffer;
}

but when I call it from main, myvec = randIntSequence(10), I see the myvector always empty. Shall I modify the return value?

Upvotes: 1

Views: 341

Answers (4)

sly
sly

Reputation: 1780

Since the question is about STL, and all you want is a vector with random entries then: 

std::vector<long int> v(10);
generate( v.begin(), v.end(), std::rand ); // range is [0,RAND_MAX]

// or if you provide long int MTRand::operator()()  
generate( v.begin(), v.end(), MTRand() );

But if you want to fix your function then

  • n should be size_t not long int
  • First loop is no-op
  • As John is saying, buffer is a pointer, so buffer[0] is your vector, and buffer[i] for i!=0 is garbage. It seems you have been very lucky to get a zero-sized vector back instead of a corrupt one!
  • Is your intention to do random shuffle? If yes, you are shuffling around zeros. If you just want to generate random entries then why don't you just loop the vector (from 0 to buffer->size(), not the other way around!!) and assign your random number?

C++ is not garbage collected, and you probably don't want smart pointers for such simple stuff, so you'll be sure to end up with leaks. If the reason is in generating a heap vector and returning by pointer is avoiding a copy for performance's sake, then my advise is don't do it! The following is the (almost) perfect alternative, both for clarity and for performance: 

vector<T> randIntSequence( size_t n ) { 
   vector<T> buffer(n);
   // bla-bla
   return buffer; 
}

If you think there is excess copying around in here, read this and trust your compiler.

Upvotes: 1

Benjamin Lindley
Benjamin Lindley

Reputation: 103693

Your question has already been answered, so I'll make this CW, but this is how your code should look.

std::vector<long int> randIntSequence(long int n)
{
    std::vector<long int> buffer(n);
    for(int i=0; i<n; ++i)
        buffer[i] = i;
    std::random_shuffle(buffer.begin(), buffer.end());
    return buffer;
}

There is absolutely no reason you should be using a pointer here. And unless you have some more advanced method of random shuffling, you should be using std::random_shuffle. You might also consider using boost::counting_iterator to initialize the vector:

std::vector<long int> buffer(
    boost::counting_iterator<long int>(0),
    boost::counting_iterator<long int>(n));

Though that may be overkill.

Upvotes: 3

John Kugelman
John Kugelman

Reputation: 361565

The swap call is indexing the *buffer pointer as if it were an array and is swapping around pointers. You mean to swap around the items of the vector. Try this modification:

swap((*buffer)[i], (*buffer)[j]);

Secondary to that, your at calls don't set the values as you expect. You are pulling out the items in the vector but not setting them to anything. Try one of these statements:

buffer->at(i) = i;
(*buffer)[i] = i;

Upvotes: 5

James McNellis
James McNellis

Reputation: 355009

You never assign to any of the elements in the vector pointed to by buffer:

for (long int i = 0; i < n; i++)
    buffer->at(i); // do you mean to assign something here?

You end up with the vector containing n zeroes.

Upvotes: 4

Related Questions