1

Here is simple code I'm asking my questions about.

struct Class{
public:
    int key;
    Class*next;
};

int main(){
Class c;
    c.key = 1;
    Class* p = &c;
    for (int i = 2; i < 5; i++){
        Class next;
        next.key = i;
        p->next = &next;
        p = p->next;
    }
    p = &c;
    for (int i = 1; i < 5; i++){
        std::cout << p->key;
        p = p->next;
    }
}

Output I was expecting: 1234

Output I've got: 1444

Can you, please, tell me what is wrong with my code, and what do I have to do to get 1234.

qIUoF7
  • 49
  • 4
  • 4
    [undefined behaviour](http://stackoverflow.com/questions/6441218/can-a-local-variables-memory-be-accessed-outside-its-scope) – chris Apr 03 '15 at 19:33
  • No debugger for you? – duffymo Apr 03 '15 at 19:33
  • A local variable is only valid up until the end of scope is reached. The end of scope is reached on each iteration of a loop. – jxh Apr 03 '15 at 19:35
  • The line `Class next'` creates a temporary which you're storing a pointer to in your linked list. This will give you undefined behaviour. – Sean Apr 03 '15 at 19:35
  • You'll need to learn about the [lifetime of objects](http://en.cppreference.com/w/cpp/language/lifetime). And from there, what a [dangling pointer](http://stackoverflow.com/questions/17997228) is. – Drew Dormann Apr 03 '15 at 19:35
  • Pointers point to things. They are just numbers. The underlying memory must not be deleted (which includes going out of scope) or your pointer starts to point to unallocated memory. – user3427419 Apr 03 '15 at 19:36
  • 1
    `Class` is a really bad name for a struct. – Keith Thompson Apr 03 '15 at 20:14

4 Answers4

2
for (int i = 2; i < 5; i++){
    Class next;
    next.key = 2;
    p->next = &next;
    p = p->next;
}

The lifetime of the next object ends at the end of each iteration. Since you're assigning the address of this object to p->next, this pointer is left dangling when the iteration ends. In the next iteration, when you attempt to use p, you are invoking undefined behaviour.

Joseph Mansfield
  • 108,238
  • 20
  • 242
  • 324
  • Ok I get it. Another question. How do I create 100000 objects and link them in a list without creating each of them in the loop? – qIUoF7 Apr 03 '15 at 19:40
  • 1
    @qIUoF7 You will still need to create them in the loop, but use dynamic allocation instead of automatic allocation. There's a lot to learn with regards to dynamic allocation, however, and experienced C++ developers aim to avoid doing it manually as much as possible. – Joseph Mansfield Apr 03 '15 at 19:42
1

Your logic seems correct but as the other commentators pointed out the variables created inside a loop are local to that loop so expire as soon as you are out of the loop. You need a minor modification.

Alternate solution: Just replace your first loop with:

for (int i = 2; i < 5; i++){                    
    p->next = new Class;
    p->next->key = i;           
    p = p->next;
}

This will allocate required memory and create a new entry at each iteration.

Hope that helps!

erol yeniaras
  • 3,701
  • 2
  • 22
  • 40
0

The problem is that inside this loop

for (int i = 2; i < 5; i++){
    Class next;
    next.key = 2;
    p->next = &next;
    p = p->next;
}

for each its iteration you are using the same local variable. Thus for the second iteration of the loop p->next points to the same local object. So your porgram has undefined behaviour because this local object is not alive after exiting the loop. In general case the memory occupied by the local variable can be overwritten.

It seems that as result you have to nodes. One is c that points to already "died" node next and the died node next that points to itself and keeps the last value that was stored in data member key that is 4.

You should either dynamically allocate each node or use an array of nodes of the list.

Here is a demonstrative program that shows how you could use a simialr approach by means of an array.

#include <iostream>

struct Class
{
public:
    int key;
    Class *next;
};

int main() 
{
    const size_t N = 4;
    Class c[N];

    c[0].key = 1;
    Class *p = &c[0];

    for ( size_t i = 1; i < N; i++ )
    {
        c[i].key = i + 1;
        p->next = &c[i];
        p = p->next;
    }

    p = &c[0];

    for ( int i = 0; i < N; i++ )
    {
        std::cout << p->key;
        p = p->next;
    }

    std::cout << std::endl;

    return 0;
}

The output is

1234
Vlad from Moscow
  • 301,070
  • 26
  • 186
  • 335
0

To explain Chris' comment - accessing a variable outside of its scope is undefined behavior. Your next variable's scope is one iteration of the for loop, once you have reached the closing } for the loop (and i++ has run, I think), it is up to the compiler to deal with deallocating the memory for next, and then reinitializing it next run. Your compiler appears to be making the reasonable decision to keep next in the same piece of memory and run the constructor again on each iteration, which is why your memory access is not throwing segmentation faults (IE, trying to access memory your program hasn't been granted access to). Your compiler could also decide to deallocate next as soon as you exit the for loop, but also doesn't appear to be doing that, which is why you are able to access its memory to print.

Long story short, don't worry too much about figuring out why your program does what it does, instead use heap memory as shown by kvorobiev, which will persist past the end of the for loop's scope. As good practice, you should also delete the memory once you are done wit it.

IllusiveBrian
  • 3,105
  • 2
  • 14
  • 17