1

I'm new to c++ and tried to get familiar with the language by implementing a LinkedList. 

class ListElement {
public:
    int val;
    ListElement *next;

    ListElement(int v, ListElement *n) {val = v; next = n;};
};

ListElement contains an int value val and a pointer to the next list element (nullptr if there is no next element) and a constructor.

class MyLinkedList {
public:
    ListElement *head;

    MyLinkedList() {head = nullptr;};

    ListElement* getHead(void){
        return head;
    };

    void append(int i) {
        head = &ListElement(i, head);
    };
};

MyLinkedList contains a pointer to the first element of the list named head as well as some methods working on the list. Encountering some bugs in those methods I tried to track down their cause. (I'm aware that a getter for a public class member makes no sense at all, originally head was private.) Doing so I observed the following behaviour I can't explain:

int main() {
    MyLinkedList l;
    l.append(1);

    int x = l.head->val;
    cout << "head: " << x << "\n";
    int y = l.getHead()->val;
    cout << "getHead(): " << y << "\n";
    int z = l.head->val;
    cout << "head: " << z << "\n";

    cin.get();
    return 0;
}

Running this code (add #include <iostream> and using namespace std; for a working example) prints

head: 1
getHead(): 18085840
head: -858993460

so the first direct access of head works just as expected, yielding 1 as value of the first list element, but using the getter returns garbage. If head is then accessed directly again, it also yields garbage, which made me think "Hm, seems like using getHead() somehow garbles the ListMember object", just to discover that

int x = l.head->val;
cout << "head: " << x << "\n";
int z = l.head->val;
cout << "head: " << z << "\n";

prints 

head: 1
head: -858993460

without even touching the getter. So is simply accessing l.head in any way enough to garble it?

No, as 

int x = l.head->val;
int z = l.head->val;
cout << "head: " << x << "\n";
cout << "head: " << z << "\n";

returns (as intended) head: 1 two times. So using cout in between changes my objects or their pointers? And what's wrong with getHead() as all it does is just return head;?

So I'm pretty lost here and couldn't find any directly related questions. (This Question has a promising title but doesn't use pointers). Am I totally failing to use pointers in a correct way? Or is some automatic object deletion going on behind the scenes? Or is this just how magiC++ works?

Community
  • 1
  • 1
Laikoni
  • 201
  • 1
  • 7
  • 2
    `head = &ListElement(i, head);` - that is not how you allocate a new `ListElement`. Go read about `new` (and then for most cases, try to avoid needing it). – user2357112 Jul 21 '16 at 22:12
  • 1
    for good measure, you might also want to know about [smart pointers](http://stackoverflow.com/questions/106508/what-is-a-smart-pointer-and-when-should-i-use-one) – jaggedSpire Jul 21 '16 at 22:15
  • @jaggedSpire Right, my bad - too late to think straight I guess :\ – jpw Jul 21 '16 at 22:24

3 Answers3

2

In

void append(int i) {
    head = &ListElement(i, head);
};

ListElement(i, head) creates a temporary, nameless ListElement and assigns a pointer to it to head. Then, because the ListElement itself wasn't assigned to anything, the ListElement goes out of scope and is destroyed. This leaves head pointing to invalid memory.

Head could be written to use dynamic memory to extent the life of the ListElement

void append(int i) {
    head = new ListElement(i, head);
};

but now someone has to pick up the responsibility of ensuring that the ListElement is deleted when no longer required.

For example:

void remove(int i) {
    // find list element i and previous element. Special handling required for first element
    prev.next = element.next;
    delete element;
};

Careful use of std::unique_ptr and std::move can get automate the memory management and eliminate the need for delete.

user4581301
  • 33,082
  • 7
  • 33
  • 54
1

change

void append(int i) {
    head = &ListElement(i, head);
};

to

void append(int i) {
    head = new ListElement(i, head);
};

The first, if it compiles, is taking the address of a temporary stack allocated object. Hence head will "point at garbage" after it's destruction.

GreatAndPowerfulOz
  • 1,767
  • 13
  • 19
1

In C++ you must manage your own memory. The statement

ListElement(i, head);

Creates an instance of ListElement locally scoped the MyLinkedList::append(). Thus once that function exits the variable no longer exists and the pointer is now pointing at invalid memory.

The reason your first print statement is giving you a seemingly correct answer is a bit of a red herring. In all cases you are accessing free-ed memory which has undefined behavior. In the first case that memory just happens to have the value that you previously set.

You have to allocate your own memory in append and clean it up when you are done with it. Once you have mastered "new" be sure to look up how to iterate through a data structure and delete each element. With your linked listed implementation this should be fairly trivial.

ruffyontheweb
  • 28
  • 3