CODEWITHSUNDEEP
c++c++11
Wilfred Hughes
Wilfred Hughes

Reputation: 31171

Keeping a pointer to the previous loop value?

I'm trying to iterate over a vector, keeping a copy of the value from the previous iteration. To my surprise, my prev pointer ends up pointing at the current value of loop.

#include <iostream>
#include <vector>

int main() {
    std::vector<std::string> v;
    v.push_back("one");
    v.push_back("two");
    v.push_back("three");

    std::string *prev = nullptr;
    for (std::string s : v) {
        std::cout << "s: " << s << std::endl;
        if (prev == nullptr) {
            std::cout << "prev: (null)" << std::endl << std::endl;
        } else {
            std::cout << "prev: " << *prev << std::endl << std::endl;
        }
        prev = &s;
    }

    std::cout << "final prev: " << *prev << std::endl;
}

Here's the output:

s: one
prev: (null)

s: two
prev: two

s: three
prev: three

final prev: three

Why is this? What's the right way to fix it?

Upvotes: 1

Views: 1903

Answers (5)

MikeMB
MikeMB

Reputation: 21156

Just change 

for (std::string s : v) {

to 

for (std::string& s : v) {

That way prev = &s; is storing the address of the actual string inside the vector instead of constructing a local copy and then taking the address of that local variable.

To elaborate a little more:
In your original version, s is a local std::string variable that gets newly copy constructed in each loop iteration. Then at the end of the loop you are taking the address of that local variable which is destroyed directly after wards, resulting in a dangling pointer and undefined behavior, when accessing that pointer in the next iteration. However, because s will usually be created at the same position in each loop iteration, the undefinded behavior will realize itself in the form of prev now pointing to the new local variable s which is already initialized with the next element of the array. Thus address stored in prev will be valid in the next cycle, but its now pointing to a fresly created local variable which is again a copy of the next element in the vector. Thus std::cout << "prev: " << *prev will usually print the same string as std::cout << "s: " << s. With certain compiler optimizations however one could also imagine other possible behaviors.

Now with the new variant, s is again a local variable, but now it's not a copy but a reference to the current element in v. As a result &s now returns the address to the actual element in the vector - and not to the local variable - whose content is not changed between loop iterations and you can use prev the way you expected.

Upvotes: 3

Antonio
Antonio

Reputation: 20296

Because you are assigning a pointer to a variable that is changing, and when that variable changes also what you read with your pointer changes.

This would work:

std::string& prev = v.front();
for (std::string& s : v) {
    std::cout << "s: " << s << std::endl;
    std::cout << "prev: " << std::endl;
    prev = s;
}

The problem is that, obviously, at the first cycle you have an inconsistent state. You should probably iterate starting from the second element of the vector, and treat in a special way the first element of your vector. That is possible for example building a range view.

Upvotes: 0

user3594141
user3594141

Reputation: 

    string *prev = nullptr;
    for(string &s : v)  //note change
    {
        if(!prev)
            cout<<s<<"-- null"<<endl;
        else
            cout<<s<<"--"<<*prev<<endl;
        prev = &s;
    }
    cout << "final prev: " << *prev << endl;

Upvotes: 0

Joseph Mansfield
Joseph Mansfield

Reputation: 110698

The scope of the s lasts until the end of the for loop. If you store a pointer to s, it is left dangling at the end of that iteration. As you are dereferencing this dangling pointer, you have undefined behaviour. It just so happens that your implementation reuses the same memory for s in each iteration, so your pointer always happens to be pointing at the current value of s.

You simply can't keep a pointer/reference to something that exists only in a particular iteration of a loop and expect it to be valid in the next iteration. The fix, in your case, is simple: don't store a pointer/reference - store the value itself by making prev a std::string. Alternatively, you can keep a pointer to the original elements of the std::vector, but that means changing s to a std::string&.

Upvotes: 2

Unda
Unda

Reputation: 1899

You're storing the address of the std::string s variable in your std::string *prev pointer. The thing is, the value of the s variable is changed to the current value extracted from v at every iteration, so that's why your prev pointer points to the current value of the loop.

A way to fix it would be to make your prev variable a simple std::string and assigning s to it at the end of every iteration. You would be saving "by value".

Upvotes: 5

Related Questions