c++ object list by pointer

Question

I want to create a list of objects, by only saving a pointer to the very next object in each object.

#include <iostream>
class A
{
    int v;
    A *next;
public:
    A(int temp):v(temp) {}
    A* createNext()
    {
        A temp(v+1);
        next = &temp;
        return next;
    }
    int getv(){return v;}
};
int main()
{
    A first(0);
    A * next = first.createNext();
    std::cout << next->getv() << "\n";
    next = next->createNext();
    std::cout << next->getv() << "\n";
}

When i execute this Programm, it gives me consistently a 1 for the first cout, but the second is a random number out of the range of an integer.

Have you considered `std::forward_list`? Also, do not store the address of function local variables, that's UB — Borgleader, Jan 23 '18 at 19:44
You are creating a temp A instance in the stack, and storing its address in next member variable. As soon you exit the createNext function, that address become garbage — Gian Paolo, Jan 23 '18 at 19:47
Why the downvotes? Here is a complete code sample with clear explanation of whats wanted and what goes wrong. Just because the answer is obvious and well known does not make it a bad question — pm100, Jan 23 '18 at 19:53

Alan · Accepted Answer · 2018-01-23T22:05:36.087

In your createNext() function, you're creating an object in stack memory, then assigning a reference to it. However, once that function exits, the stack frame will be cleared and the next pointer will reference a garbage value.

A* createNext()
{
    A temp(v+1); // this creates an object on the stack
    next = &temp;
    return next;
}

Instead, you'll need to create the object on the heap, and return it. Heap memory is not tied to a function, and can be accessed outside of the scope in which it was allocated.

A* createNext()
{
    next = new A(v+1); // this creates an object on the heap
    return next;
}

However, if you allocate objects on the heap (i.e. using new) you will need to deallocate them afterwards (i.e using delete). Failing to do so creates a so called "memory leak."

Using more modern features of C++, you can mitigate some of the pitfalls of memory management. Though given the nature of your question, you should strive to have a solid understanding of memory allocating in C++ before using those abstractions.

I think you may either want to wrap the list in an owning outer handle or use `std::shared_ptr`. There are other ways of course — Aluan Haddad, Jan 23 '18 at 21:23

score 2 · Answer 2 · answered Jan 23 '18 at 19:46

With

    A temp(v+1);
    next = &temp;
    return next;

you create a "local" variable, i.e. one with automatic storage duration which's life time is ending at the end of the function, and then return its address. So you will return the address of an object that has gone out of scope, which is undefined behaviour.

Without inspecting your logic to much, and just focussing on fixing this memory issue, probably you'll have to write

    A* temp = new A(v+1);
    next = temp;
    return next;

score 0 · Answer 3 · answered Jan 23 '18 at 19:46

0

   A* createNext()
    {
        A temp(v+1);
        next = &temp;
        return next;
    }

You're returning a pointer to a local object temp, this will go out of scope and the pointer will be dangling. All your dereferences from then on are undefined behavior. Consider using std::unique_ptr in combination with shared. Or just use the standard provided std::forward_list.

answered Jan 23 '18 at 19:46

Hatted Rooster

35,759
6
62
122

your text suggests that unique_ptr will solve the local going out of scope issue. It will not. I dont think you mean that , but thats how it reads – pm100 Jan 23 '18 at 19:49
@pm100 rather the member. – Hatted Rooster Jan 23 '18 at 19:49

c++ object list by pointer

3 Answers3