C++ strange segmentation fault

Question

I am getting a strange segmentation fault in my program. Dlist is a class that creates a linked list with operations to insert and remove items from a dynamic list. I am positive my implementation of this class is correct, but this code is producing a seg fault. The strange part is, when I make my atleastTwo and atleastOne functions pass by reference, the seg fault disappears and everything compiles. Can anyone shed some light on this problem?

bool atleastTwo(Dlist<double> stack){
try{
    stack.removeFront();
    stack.removeFront();
} catch(emptyList e){
    cout << "Not enough operands\n";
    return false;
}
return true;
}

bool atleastOne(Dlist<double> stack){
try{
    stack.removeFront();
} catch(emptyList e){
    cout << "Not enough operands\n";
    return false;
}
return true;
}

void processInput(inputs usrInput, Dlist<double> &stack){
switch(usrInput){
    case i_add:
        if(atleastTwo(stack)){doOperation(stack, add);}
        break;
    case i_subtract:
        if(atleastTwo(stack)){doOperation(stack, subtract);}
        break;
    case i_multiply:
        if(atleastTwo(stack)){doOperation(stack, multiply);}
        break;
    case i_divide:
        if(atleastTwo(stack)){doOperation(stack, divide);}
        break;
    case i_negation:
        if(atleastOne(stack))negation(stack);
        break;
    case i_duplicate:
        if(atleastOne(stack)){duplicate(stack);}
        break;
    case i_reverse:
        if(atleastTwo(stack)){reverse(stack);}
        break;
    case i_print:
        if(atleastOne(stack)){print(stack);}
        break;
    case i_clear:
        clear(stack);
        break;
    case i_printAll:
        printAll(stack);
        break;
    default:
        break;
}
}

T *removeFront();
// MODIFIES this
// EFFECTS removes and returns first object from non-empty list
//         throws an instance of emptyList if empty

Thanks

Do you follow the rule of three/five/whatever? http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three — chris, Dec 11 '12 at 05:10
By your description, the problem lies with copying the Dlist. We need to see the implementation of Dlist to pinpoint the issue (I suspect it's related to pointer copying). — Andrei Tita, Dec 11 '12 at 05:11
Yeah I have followed the rule of 3. In fact, when I run [this code here](http://pastie.org/5509559) there are no problems with compilation. (same implementation of Dlist). The difference is this is just an isolation of my atleastTwo function. There are no other functions in the file. — nan, Dec 11 '12 at 05:17
@user1693091, Is there any chance we can see the implementation? Also, are you compiling with C++11? If so, you must follow the Rule of Five instead. If you can, use RAII instead of following any of these outdated rules. — chris, Dec 11 '12 at 05:20
@user1693091 In your new code, you are not accessing the stack again after calling the function on it. — Andrei Tita, Dec 11 '12 at 05:22
atleastOne(Dlist stack) here stack is a copy of your original stack. — ray_linn, Dec 11 '12 at 05:29
Can't you just use a stack implementation that reports its size? Copying, then try/catching to determine the size is ridiculous. — Alexander Kondratskiy, Dec 11 '12 at 05:32
[Dlist implenentation](http://pastie.org/private/tduq2kfxtvgnm7chjbtrvw) No I'm not using C++11. Yeah the problem here really boils down to me not understanding entirely well how the whole deep/shallow copy works. I didn't know it was that bad to copy a list. — nan, Dec 11 '12 at 05:34

score 1 · Accepted Answer · answered Dec 11 '12 at 05:46

Regarding the question itself I do not see how a segfault could occur from your code. I suspect the issue being in the code for Dlist, maybe a badly implemented destructor?

To solve your problem you could implement a count of elements inside Dlist and check for it. But maybe you are not allowed to modify Dlist. The preferred solution to avoid jumpy code and too much tests would be following suggestion. Instead of testing for the amount of operands just try it, and put the exception handler inside your processing method. Problem with this second solution is that the stack might remain in an inconsistent state: that means you cannot proceed with computations and should restart from the beginning.

void processInput(inputs usrInput, Dlist<double> &stack)
{
  try{
    // .... your old code WITHOUT ifs
  } catch(emptyList e){
    cout << "Not enough operands\n";
  }
}

I suppose the latter is a way to go, you could keep a copy of your stack and make the computation on one copy. The performance would be much better this way and above that the code easier to read and understand.

I hope it helps.

Thanks. This was a better way to do this. – nan Dec 11 '12 at 07:06 — nan, Dec 11 '12 at 07:06

score 0 · Answer 2 · answered Dec 11 '12 at 05:15

0

If you pass by value (not using reference) then you are effectively making a copy each time. Exactly how that is confusing things isn't clear, you'd need to study exactly what the various copy constructors are doing. But surely passing by reference is what you mean? Why would you copy a whole collection?

answered Dec 11 '12 at 05:15

djna

54,992
14
74
117

The problem I'm having is this implementation of atleastTwo/One needs to not alter the list in anyway (but at the same time check if there are two elements in it). I couldn't seem to figure out a way to pass by reference and solve this problem. – nan Dec 11 '12 at 05:19

Andrei Tita · Answer 3 · 2012-12-11T06:25:56.713

After staring at your Dlist implementation code for quite a while (doesn't look wrong at a first glance), I think the problem is here:

template <typename T>
Dlist<T>::Dlist(const Dlist &l){
    removeAll();
    copyAll(l);
}

Calling removeAll() before initializing first (and last) to NULL will lead to bad things, given the operations which are performed there.

Another problem with the Dlist design is that you are holding raw pointers (which is tricky but not necessarily bad, assuming it's clear to everyone that the user is responsible for deleting them), yet you have implemented deep copy mechanics. Temporary Dlist objects will leak, as will the copies you are passing to your atLeastOne and atLeastTwo functions, unless you manually delete their contents.

C++ strange segmentation fault

3 Answers3