Why another branch is unreachable in my code?

Question

Why the output of the following code is always suck. How to get happy as the output? Why the happy branch is unreachable?

public class HowToMakeStackoverflowBetter {

    private static final int HUMAN_PATIENCE = 10;
    private List<Member> members = new ArrayList<>();
    private int atmosphere = -10;
    private Random r = new Random();
    public HowToMakeStackoverflowBetter(int size) {
        for (int i = 0; i < size; i++) { members.add(new Member()); }
    }
    public Member pick() { return members.get(r.nextInt(members.size())); }

    public class Member {
        private int patience = HUMAN_PATIENCE;
        private Question question = null;
        public Member() { patience = r.nextInt(patience+1) + atmosphere; }
        public void vote(Question q) {
            if (patience >= 0) {
                voteUp(q);
            } else {
                voteDown(q);
            }
        }
        public void ask() {
            question = new Question();
            for (Member member : members) {
                member.vote(question);
            }
        }
        private void voteUp(Question q) { ++q.vote; }
        private void voteDown(Question q) { --q.vote; }
        public String toString() {
            return (question.vote >= 0)? "Happy!" : "Suck!";
        }
    }

    public class Question { private int vote; }

    public static void main(String[] args) {
        HowToMakeStackoverflowBetter stackoverflow = new HowToMakeStackoverflowBetter(100);
        Member me = stackoverflow.pick();
        me.ask();
        System.out.println(me);
    }
}

After a 1000 times loop, it gives us 1000 sucks. I remember 2 or 3 years ago, this was not the case. Something changed.

Tony Delroy · Accepted Answer · 2013-12-30T05:52:27.057

Two problems. First:

linkedList::linkedList(){  
    *sentinel.last=sentinel;  
    *sentinel.next=sentinel;  
    sentinel.str="I am sentinel!!";  
};

sentinel is your member variable, and .last is its pointer to another node. This hasn't been initialised, so trying to use it is undefined behaviour. In practice, it's effectively pointing at a random address in (or out of) memory, and you attempt to dereference the pointer then copy the entire sentinel object over the node at the imagined pointed-to address: i.e. you try to copy the 3 pointers in the sentinel node member variable to a random address in memory.

You probably want to do this:

linkedList::linkedList()
{  
    sentinel.last = &sentinel;  
    sentinel.next = &sentinel;  
    sentinel.str = "I am sentinel!!";  
}

Secondly, you explicitly call the destructor for linkedList, which results in undefined behaviour when the compiler-arranged destruction is performed as the object leaves the stack scope it's created in - i.e. at the end of main().

I suggest you change node.str to be a std::string, as in any realistic program you'll want to be able to handle variable text, and not just point to (constant) string literals. As is, if you mix string literals and free-store allocated character arrays, you'll have trouble knowing when to call delete[] to release the memory. You could resolve this by always making a new copy of the string data to be stored with new[], but it's safer and easier to use std::string.

thx very much. that's the problem!. It work well after I change these two line `*sentinel.last=sentinel; *sentinel.next=sentinel;` to `sentinel.last=&sentinel; sentinel.next=&sentinel;`. But I m still a confused what's the difference between these two kinds of code? — shen, Dec 30 '13 at 05:57
@weiShen: well, you might want to read my answer here: http://stackoverflow.com/questions/4955198/what-does-dereferencing-a-pointer-mean/4955297#4955297 - then consider that the `*sentinel.next=` code is not modifying the `.next` pointer, but the memory at the address the current `.next` pointer value points at (which is an uninitialised/garbage address). Hope that helps! — Tony Delroy, Dec 30 '13 at 07:20

Digital_Reality · Answer 2 · 2013-12-30T05:41:39.033

1

You forgot to initialize sentinel

In code below you are trying to initialize sentinel (which is not yet constructed) with sentinel(same thing). So you have to pass something to constructor which can be used to initialize your member variable sentinel

*sentinel.last=sentinel;

Also no need to call destructor like this. Destructor will be called once your myList goes out of scope.

myList.~linkedList();

edited Dec 30 '13 at 05:41

answered Dec 30 '13 at 05:35

Digital_Reality

4,488
1
29
31

thx. but how to initialize it? I ve already declare it in header file. – shen Dec 30 '13 at 05:37
must I creat a new instance of sentinel like: `sentinel=new node();`? – shen Dec 30 '13 at 05:47

score 1 · Answer 3 · answered Dec 30 '13 at 05:36

1

Since you allocated it as a local variable, your mylist will be destroyed automatically upon exiting main. Since you've already explicitly invoked its destructor, that leads to undefined behavior (attempting to destroy the same object twice).

As a quick guideline, essentially the only time you explicitly invoke a destructor is in conjunction with placement new. If you don't know what that is (yet), that's fine; just take it as a sign that you shouldn't be invoking destructors.

answered Dec 30 '13 at 05:36

Jerry Coffin

476,176
80
629
1,111

ok. but why still doesn't work when I delete this line `myList.~linkedList();`? – shen Dec 30 '13 at 05:45
You also need to initialize `sentinel` properly, something like: `sentinel.next=&sentinel; sentinel.last=&sentinel;` (which assumes you're creating a circular linked list). Right now, you're writing to whatever random address `sentinel`'s pointers happen to contain. – Jerry Coffin Dec 30 '13 at 05:55
that's it. THX:) but what's the difference between these two expression? `sentinel.next=&sentinel;` and `*sentinel.next=sentinel;`? – shen Dec 30 '13 at 06:08
@weiShen: `sentinel.next = &sentinel;` takes the address of sentinel and stores it into `sentinel.next`. `*sentinel.next = sentinel;` takes the value of sentinel and stores it into the address pointed to by `sentinel.next`. – Jerry Coffin Dec 30 '13 at 06:12
does it means `*sentinel.next = sentinel;` will totally copy everything of sentinel and store them in another adresse in memory and then store this adresse in sentinel.next? so the difference is `*sentinel.next = sentinel;` will copy another instance, but `sentinel.next = &sentinel;` not? – shen Dec 30 '13 at 06:19
@weiShen: No -- `*sentinel.next =sentinel;` copies all of `sentinel` to another address in memory, but does *not* modify `sentinel.next` -- it just uses whatever address `sentinel.next` happened to contain (some arbitrary number) and writes there. – Jerry Coffin Dec 30 '13 at 06:25
thanks for your patience. I ll vote for your answer when I have enough reputation. – shen Dec 30 '13 at 06:43

score 0 · Answer 4 · answered Dec 30 '13 at 05:36

0

the program may crash, with this:

    *sentinel.last=sentinel;  
    *sentinel.next=sentinel;

sentinel is not initialized sot i has random value on stack.

answered Dec 30 '13 at 05:36

tristan

4,235
2
21
45

score 0 · Answer 5 · answered Dec 30 '13 at 05:49

You're trying to de-reference the pointers last and next of member variable sentinel when they are not yet initialized.
And these de-references *sentinel.last=sentinel *sentinel.next=sentinel are causing the crash because without assigning the values to pointers you're changing the value pointed by the pointers.

You can do like this

sentinel.last=&sentinel;  
sentinel.next=&sentinel;

And as pointed out by other explicit destructor calls aren't need here.

Why another branch is unreachable in my code?

5 Answers5