0

I recently ran into an issue while experimenting with linked lists. When I use a function to link up a new node that has a string in its data field it doesn't work. By this I mean that when the function (linkin() see below) returns , the string (which was local to the function) is destroyed and so the string field appears to be uninitialized.

However, when I do this exact same operation with an int it seems to work just fine. The code I used is below (its the int version, but make val a string instead of an int to see the other version). Could someone explain to me what is happening?

Thanks!

struct testlist {
    int val;
    testlist *next;
};
void linkin ( testlist *a );

int main() {

    testlist test;

    linkin(&test);
    cout << test.next->val <<endl;
}


void linkin ( testlist *a )
{
  testlist b;
  b.val=1;
  a->next = &b;
  cout << a->next->val <<endl;
}
Improve this question
10
  • 2
    Oh, well, thank you for showing us the code that is working and hiding the one that is broken. We can certainly guess what is wrong with the code we haven't seen by looking at different code that works. Commented Oct 29, 2013 at 15:35
  • When you say string, do you mean std::string or char *? Commented Oct 29, 2013 at 15:35
  • Actually, nevermind. This code is broken too. It just works by accident. See stackoverflow.com/questions/6441218/… Commented Oct 29, 2013 at 15:35
  • 1
    a->next = &b; is wrong! &b will be invalid after the linkin() function exits! Commented Oct 29, 2013 at 15:36
  • That code throws an automatic variable into a linked list, then attempts to reference it outside the scope of its demise. The term "working" is in the mind of the undefined-behavior-invoking beholder. Commented Oct 29, 2013 at 15:36

2 Answers 2

Reset to default
4 
testlist b;
a->next = &b;

a->next is pointing to a local temporary object which will be destroyed right after returning from function. It invokes undefined behavior after dereferencing it out of the function.

It's undefined behavior, sometimes it works, sometimes not.

Also in C++ there is a linked-list: std::list. On the other hand, you can use smart-pointers such as std::unique_ptr instead of bare pointers. I've written a smart-pointer based of your container:

struct List
{
    int val;
    unique_ptr<List> next;
};

void linkin (List &a)
{
    unique_ptr<List> b(new List);
    b->val = 1;
    a.next = move(b); // After move you can't use b anymore
    cout << a.next->val << endl;
}

int main()
{
    List test;
    linkin(test);
    cout << test.next->val <<endl;
}
Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

0

Straight away, in linkin() I can see you're storing a pointer in a to an object that is going to go out of scope - ie: be destroyed.

There are reasons why this might appear to work with an int but not a complex type like a std::string. Despite appearances this is broken code - try rewriting linkin():

void linkin ( testlist& a )
{
   testlist* b = new testlist;
   b->val=1;
   a->next = b;
   cout << a->next->val <<endl;
}
Improve this answer

5 Comments

Not so sure if using new without a smart pointer is a really good advice!
@g-makulik You simply, absolutely, categorically, fundamentally, cannot be frickin' serious.
I'am serious about this!
Have a look at MM's answer to see what I mean. Again: I am serious about this point, and what you show in your answer is not good advice!
I'm never going to agree with the idea that good coding must involve assuming oneself incapable of managing heap memory. There are some occasions where use of various smart pointers can be very helpful - heap allocation inside a try block for example - but they are certainly not a necessity in a simple linked list. Further to that, my answer illustrated perfectly why his code didn't work and why, that he needed to use the heap, managing it from there is up to the questioner

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.