Why is my code getting a segmentation error

Question

For the life of me I cannot understand at all why this program is getting a segmentation error. The issue is that it retrieves an object within the vector container uses a function within the menu class using the get_command() and for some reason after testing the main function line by line this one results in a segmentation fault:

menu->get_command()->execute();

I have tried changing the syntax to create a new command object that stores the returned object from get_command() and changed the index between 0 and -1 and still nothing fixes the error. I have spent at least a couple of hours trying to figure out why but I cannot seem to find a solution.

class Base {
public:
    /* Constructors */
    Base() { };

    /* Pure Virtual Functions */
    virtual double evaluate() = 0;
    virtual std::string stringify() = 0;
};

class op : public Base
{
public:
    op() { };
    op(double op1) { operand = op1; }

    double evaluate() { return operand; }

    string stringify() {
        string value = to_string(operand);
        return value;
    }
private:
    double operand;
};

class Command {
protected:
    Base* root;

public:
    Command() { this->root = nullptr; }
    double execute() { return root->evaluate(); }
    std::string stringify() { return root->stringify(); }
    Base* get_root() { return root; }
};



class Menu {
private:
    int history_index; // Indexes which command was last executed, accounting for undo and redo functions
    std::vector<Command*> history; // Holds all the commands that have been executed until now

public:
    Menu() {
        // Constructor which initializes the internal members
        history_index = -1;
    }

    std::string execute() {
        // Returns the string converted evaluation of the current command
        return to_string(history[history_index - 1]->execute());
    }

    std::string stringify() {
        // Returns the stringified version of the current command
        return history[history_index]->stringify();
    }

    bool initialized() {
        // Returns if the history has an InitialCommand, which is necessary to start the calculation
        if (history[history_index] != nullptr)
            return true;
        else
            return false;
    }

    void add_command(Command* cmd) {
        // Adds a command to the history (does not execute it), this may require removal of some other commands depending on where history_index is
        history.push_back(cmd);
        history_index++;
    }

    Command* get_command() {
        // Returns the command that the history_index is currently referring to
        return history[history_index];
    }

    void undo() {
        // Move back one command (does not execute it) if there is a command to undo
        history_index--;
    }

    void redo() {
        // Moves forward one command (does not execute it) if there is a command to redo
        history_index++;
    }
};


class InitialCommand : public Command {
protected:
    Base* root;

public:
    InitialCommand(Base* b) { this->root = b; }
    double execute() { return root->evaluate(); }
    std::string stringify() { return root->stringify(); }
    Base* get_root() { return root; }
};


void main()
{
    Menu* menu = new Menu();
    InitialCommand* temp = new InitialCommand(new op(7));
    menu->add_command(temp);
    EXPECT_EQ(menu->get_command()->execute(), 7);

    system("PAUSE");

}

Answer 1

You're not doing inheritance right, as you are duplicating fields between Command and InitialCommand that lead to the error.

Both command classes have a Base *root member, and non-virtual execute methods. When you construct a new InitialCommand object, the InitialCommand::root object points at the op that was created for it, while Command::root remains NULL because of the default constructor for Command. Then, when you call menu->get_command(), it will call Command::execute because execute is non-virtual and menu is a Command *. Command::execute will then dereference a NULL root, causing your segmentation error.

Remove the Base *root member from InitialCommand, and pass the parameter to a constructor in Command. You probably want to make some methods like execute virtual.

Answer 2

The problem is that your Command and InitialCommand both have root variable. InitialCommand* temp = new InitialCommand(new op(7)); will according to your constructor set InitialCommand::root. So Command::root remains uninitialized. Then Menu holds std::vector<Command*>, so InitialCommand* is implicitly converted to Command*. At alst calling Command::execute will indeed call Command:execute because the method is not virtual. So, the uninitialized Command::root is used -> seg. fault.

Please don't use new. Use smart pointers - std::unique_ptr should be the default way to manage dynamic memory.

That said, your code seems too Java/C# like. This is C++, use value semantics if you can. There's no reason for Menu* menu = new Menu();. Menu menu; is simpler and works the same in your case. Here's a code I would've written

#include <memory>
#include <vector>
#include <string>

using namespace std;//Not a good practice and definitely a big no in header files.
class Base {
public:
    /* Constructors */
    Base() { };

    /* Pure Virtual Functions */
    virtual double evaluate() = 0;
    virtual std::string stringify() = 0;
};

class op : public Base
{
public:
    op() { };
    op(double op1) { operand = op1; }

    double evaluate() { return operand; }

    string stringify() {
        string value = to_string(operand);
        return value;
    }
private:
    double operand;
};

class Command {
protected:
    std::unique_ptr<Base> root;

public:
    Command(std::unique_ptr<Base>&& root):root(std::move(root)) { }
    //Be const-correct
    double execute() const { return root->evaluate(); }
    std::string stringify() const { return root->stringify(); }
    Base* get_root() const { return root.get(); }
};


class Menu {
private:
    int history_index; // Indexes which command was last executed, accounting for undo and redo functions
    std::vector<std::unique_ptr<Command>> history; // Holds all the commands that have been executed until now

public:
    Menu() {
        // Constructor which initializes the internal members
        history_index = -1;
    }

    std::string execute() const{
        // Returns the string converted evaluation of the current command
        return to_string(history[history_index - 1]->execute());
    }

    std::string stringify() const{
        // Returns the stringified version of the current command
        return history[history_index]->stringify();
    }

    bool initialized() const{
        // Returns if the history has an InitialCommand, which is necessary to start the calculation
        if (history[history_index] != nullptr)
            return true;
        else
            return false;
    }

    void add_command(std::unique_ptr<Command>&& cmd) {
        // Adds a command to the history (does not execute it), this may require removal of some other commands depending on where history_index is
        history.emplace_back(std::move(cmd));
        history_index++;
    }

    Command* get_command() const {
        // Returns the command that the history_index is currently referring to
        return history[history_index].get();
    }

    void undo() {
        // Move back one command (does not execute it) if there is a command to undo
        history_index--;
    }

    void redo() {
        // Moves forward one command (does not execute it) if there is a command to redo
        history_index++;
    }
};


class InitialCommand : public Command {
protected:

public:
    InitialCommand(std::unique_ptr<Base>&& b): Command(std::move(b)){}
};

// There's no such thing as void main
int main()
{
    Menu menu;
    auto temp = std::make_unique<InitialCommand>(std::make_unique<op>(7));
    menu.add_command(std::move(temp));
    //EXPECT_EQ(menu.get_command()->execute(), 7);

    system("PAUSE");

}

It uses move semantics which used to not be a beginners concept, but it's such integral part of modern C++ that every C++ programmer must learn it sooner rather than later.