C++, help identifying seg fault

sweenish

Diamond Member
May 21, 2013
3,656
60
91
I've been mildly abusing you guys since this is my second question in as many weeks, but I do appreciate the help.

This is an assignment, I'm supposed to take a fully parenthesized infix equation and convert it to postfix and then calculate it.

So, input takes the form: (2 + 3) + 5 OR ((2 + 3) - (3 * 1)) + 4, etc.

I am hitting a segmentation fault as my make_postfix function ends, but I cannot identify what I'm doing wrong.

Relevant code attached:
Calculator.cpp
Code:
#include <iostream>
//#include <cstring>
#include "stack.hpp"
//#include "utils.hpp"

void make_postfix(char in[]);


int main(void)
{
    using namespace std;

    char temp;             // grabs characters as the user inputs them
    int i, length;         // loop counter, counts length of equation input
    char express[50];       // holds the infix expression, without spaces


    cout << "Enter an expression in infix notation: ";
    do {
       cin.get(temp);
       if (temp != ' ' && temp != '\n') {
           express[length] = temp;
           length++;
       }
    } while(temp != '\n');
    express[length] = '\0';

    cout << "You typed: ";
    for (i = 0; i < length; i++) {
        cout << express[i];
    }
    cout << endl;

    make_postfix(express);

    cout << "Postfixed: ";
    for (i = 0; i < length; i++) {
        cout << express[i] << " ";
    }
     
    return 0;
}


void make_postfix(char in[])
{
    using namespace std;

    Stack<char> ops;            // holds operators
    char temp[50];              // holds interim postfix expression
    char scrap;                 // receives the popped '('
    int i = 0, j = 0;           // loop counter and index marker, respectively
    

    do {
        cout << "Checking(j=" << j << "): " << in[j] << endl;
        switch(in[j]) {
            case ')':
                do {
                    if (ops.top() != '(') {
                        temp[i] = ops.pop();
                        cout << "Popped: " << temp[i] << endl;
                        i++;
                    }
                } while(ops.top() != '(');
                
                if (ops.top() == '(') {
                    scrap = ops.pop();
                    cout << "Scrapped: " << scrap << endl;
                }
                break;
            case '+':
            case '-':
            case '*':
            case '/':
            case '(':
                ops.push(in[j]);
                cout << "Pushed: " << ops.top() << endl;
                break;
            default:
                temp[i] = in[j];
                cout << "Added(i=" << i << ")(j=" << j << "): " << temp[i] << endl;
                i++;     
        }
        j++;
    } while(in[j] != '\0');
    
    do {
        temp[i] = ops.pop();
        i++;
    } while(!ops.is_empty());

    temp[i] = '\0';

    cout << "Finished processing." << endl;

    for (j = 0; j < i; j++) {
        in[j] = temp[j];
        cout << "Moved: " << in[j] << " at position " << j << "." << endl;
    }
}
stack.hpp
Code:
#ifndef _STACK_
#define _STACK_

// Type definitions
template <class Element>
struct list_element
{
    Element value;
    list_element<Element> *next;
};

template <class Element>
class Stack
{
    public:
    Stack();
    ~Stack();
    Element top();
    Element pop();
    void push(Element e);
    bool is_empty();
    
    private:
    list_element<Element> *m_first;
};


// Content that would normally go in the source file: Class implementation
// (Needs to be here because it is a template class.)
// constructor
template <class Element>
Stack<Element>::Stack()
{
    m_first = NULL;
}


// destructor
template <class Element>
Stack<Element>::~Stack()
{
    list_element<Element> *prev;
    do {
        prev = m_first;
        m_first = m_first->next;
        delete prev;
    } while(m_first->next != NULL);

    delete m_first;
}


// returns the top element of the stack, but doesn&#8217;t change the stack
template <class Element>
Element Stack<Element>::top()
{
    return m_first->value;
}


// returns what was the top element of the stack, and also removes this element from the stack
template <class Element>
Element Stack<Element>::pop()
{
    Element temp;
    list_element<Element> *prev;

    if (!is_empty()) {
        temp = m_first->value;
        prev = m_first;
        m_first = prev->next;
        delete prev;
    }

    return temp;
}


// adds element to top of stack
template <class Element>
void Stack<Element>::push(Element e)
{
    list_element<Element> *n;
    n = new list_element<Element>;

    n->value = e;

    if (is_empty()) {
        m_first = n;
        n->next = NULL;
    }
    else {
        n->next = m_first;
        m_first = n;
    }

    return;
}


// returns true if stack is empty, or false otherwise
template <class Element>
bool Stack<Element>::is_empty()
{
    if (m_first) {
        return false;
    }
    else {
        return true;
    }
}

#endif
The code has other problems, like throwing a bash error if I try to divide with '/', and leaving out the final operator if it's missing the outermost ( ). But all I really need assistance with is the segmentation fault.

It correctly converts to postfix. All the cout statements in the function show that much of it working.

Any help is appreciated. The code does compile cleanly using g++ -Wall, but like I said, segmentation fault.
 
Last edited:

cabri

Diamond Member
Nov 3, 2012
3,616
1
81
I can see one potential problem off the bat.
temp is not protected from exceeding the 50 count limit
 

sweenish

Diamond Member
May 21, 2013
3,656
60
91
Good point, I'll have to make sure both my character arrays can't exceed their limits.
 

mundane

Diamond Member
Jun 7, 2002
5,603
8
81
For hunting down segfaults in general, I'd recommend either using a debugger (either provided through an IDE or gdb) and/or installing a segfault handler to provide a stacktrace (link)
 

sweenish

Diamond Member
May 21, 2013
3,656
60
91
It's a little sad that these tools aren't taught from the get-go.

It looks like the function is fine. The problem lies in my stack destructor.

Since I'm just using vim (gvim specifically) to write my code, I installed gdb.

Program received signal SIGSEGV, Segmentation fault.
0x00000000004010f8 in Stack<char>::~Stack() ()

At least now I have a place to look.

Thank you very much, cabri and mundane, for the help. I think I can go from here.

EDIT: To clarify the error, in case this might help someone, my destructor was assuming the stack wasn't empty. However, I clear the stack in my function and so only m_first needs to be deleted. By placing an if check around my block that deletes a stack with many elements, it fixed the segfault.

EDITx2: My destructor was super screwed up. I got some help from the professor about how it should look. I was getting another segfault from the destructor when it actually had values to clear. But now the whole program is working. Again, thanks for the help.
 
Last edited:

Cogman

Lifer
Sep 19, 2000
10,277
125
106
What are the rules on using the STL?

I'm guessing no, but if you are allowed to use it I would look into shared pointers, std::array, and vectors. Heck, even if you aren't allowed to use it, you might find some benefit in using std::array to get things right and then swap in raw arrays when you are satisfied with the results.

Why std::array? Because the at() method on it does bounds checking for you. You will get a much nicer error message right when you start traipsing through memory you don't own.
 
Last edited:

purbeast0

No Lifer
Sep 13, 2001
52,653
5,551
126
this should be stickied as a "HOW TO POST ASSIGNMENT HELP THREADS" on here.

glad you figured it out on your own as well OP, that's part of the fun/challenge in coding.
 

Cogman

Lifer
Sep 19, 2000
10,277
125
106
this should be stickied as a "HOW TO POST ASSIGNMENT HELP THREADS" on here.

glad you figured it out on your own as well OP, that's part of the fun/challenge in coding.

Agreed. OP did everything right when asking for help here.
 

Ken g6

Programming Moderator, Elite Member
Moderator
Dec 11, 1999
16,132
3,620
65
Well, I'm not stickying this, but I did link to it from the "use code tags" post. :thumbsup:

The next step would be a "forum rules" or "forum suggestions" post which would link to this and replace the code tags post. I'm thinking about it. :hmm:
 

sweenish

Diamond Member
May 21, 2013
3,656
60
91
What are the rules on using the STL?

I'm guessing no, but if you are allowed to use it I would look into shared pointers, std::array, and vectors. Heck, even if you aren't allowed to use it, you might find some benefit in using std::array to get things right and then swap in raw arrays when you are satisfied with the results.

Why std::array? Because the at() method on it does bounds checking for you. You will get a much nicer error message right when you start traipsing through memory you don't own.

Rules in that class were to use what you knew, as long as it was C++. The class is more about data structures than coding. So the assignment is more concerned with a correct implementation of a stack.

I don't think I could teach myself enough about those classes/methods in time to tune my assignment, but I will definitely look into them. I'm all about better practices and especially nicer error messages. I've at least heard of vectors, and they sound cool.
 

Cogman

Lifer
Sep 19, 2000
10,277
125
106
Rules in that class were to use what you knew, as long as it was C++. The class is more about data structures than coding. So the assignment is more concerned with a correct implementation of a stack.

I don't think I could teach myself enough about those classes/methods in time to tune my assignment, but I will definitely look into them. I'm all about better practices and especially nicer error messages. I've at least heard of vectors, and they sound cool.

So the great thing about these classes and methods is you already know how to use them. Array and unique/shared pointer can pretty much be dropped right into place of c style arrays and pointers with little to no code modifications.
 

sweenish

Diamond Member
May 21, 2013
3,656
60
91
Sounds good, I'll look into implementing them in my next assignment. I think it's binary trees.

I'm assuming that STL is the standard library? I used <cstdlib> once to return SUCCESS or FAIL on a previous assignment. So, shared pointers, std::array, and vectors. Good to know.