0

So I implemented a standard linked list and it compiled and ran perfectly in clang. However, g++ threw a lot of errors and after removing common memory leaks, there's one error that no book/documentation/tutorial is helping with-

struct token{
    string data;
    string id = "n";
    void print(){
        cerr<<"| "<<data<<" | ";
    }
};

struct node{
    token data;
    node *next = nullptr;
    void print(){
        cerr<<" ||| "<<this<<" | "<<data.data<<" | "<<next<<" ||| ";
    }
};

class list{
    friend class queue;
    friend class stack;
public:
    node *head = nullptr;
    node *tail = nullptr;

void insert(token incoming_data){
    if (head == nullptr){
        node *new_node = new node;
        new_node->data = incoming_data;
        new_node->next = nullptr;
        head = new_node;
        tail = new_node;
        new_node = nullptr;
    }
    else{
        node *new_node = new node;
        new_node->data = incoming_data;
        tail->next = new_node;
        new_node->next = nullptr;
        tail = new_node;
        new_node = nullptr;
    }
}

void del(){
            cerr<<"List before deletion is ";
            print();
            cerr<<"\n";
            cerr<<"Head is at "<<head;
            cerr<<"\n";
    if (head == nullptr){
                cerr<<"List is empty\n";
        return;
    }
    else if (head->next == nullptr){
        tail = nullptr;
                    cerr<<endl<<"Deleting object at "<<head<<endl;
                    delete head;
        head = nullptr; //keep an eye on this

    }
    else{
        node *temp = head;
        head = temp->next;
                cerr<<endl<<"Deleting object at "<<temp<<endl;
        delete temp;
        temp = nullptr;
    }
            cerr<<"List after deletion is ";
            print();
    cerr<<"\n";
}

~list(){
    cerr<<"Destructor calling delete"<<endl;
    while (not(head == nullptr)){
        del();
    }
}
void insert_front(token incoming_data){
    if (head == nullptr){
        node *new_node = new node;
        new_node->data = incoming_data;
        new_node->next = nullptr;
        head = new_node;
        tail = new_node;
        new_node = nullptr;
    }
    else{
        node *new_node = new node;
        new_node->data = incoming_data;
        new_node->next = head;
        head = new_node;
        new_node = nullptr;
    }
}

};

Now, it works perfectly on its own. The stacks and queues implemented using it works perfectly. However, at some point down the line when the destructor tries to call it, this is what happens-

Destructor calling delete

List before deletion is 
 ||| 0x100409bc0 | 55 | 0x100431890 |||  -->  ||| 0x100431890 | 5 | 0x100504830 |||  -->  ||| 0x100504830 | + | 0x0 |||  --> NULL

Head is at 0x100409bc0
Deleting object at 0x100409bc0
test shunting yard (76600,0x10039e380) malloc: *** error for object 0x100409bc0: pointer being freed was not allocated

Node is printed in the format ||| Address of this node | Data | Address of next node ||

And yes, every node was created dynamically using 'new'. Also, the same destructor and del functions work multiple times within the same program perfectly, yet for one particular instance, fail.

In other stack overflow question with the same error, the pointer wasn't referring to anything, however here clearly there's an object that can be deleted at the pointer in question.


Edit: It was an implementation of RPN parse using Shunting-Yard, and here's the complete code-

    #include <iostream>
    #include <string.h>
    #include<cmath>
    using namespace std;
    struct token{
        string data;
        string id = "n";
        void print(){
            cerr<<"| "<<data<<" | ";
        }
    };

    struct node{
        token data;
        node *next = nullptr;
        void print(){
            cerr<<" ||| "<<this<<" | "<<data.data<<" | "<<next<<" ||| ";
        }
    };

    class list{
        friend class queue;
        friend class stack;
    public:
        node *head = nullptr;
        node *tail = nullptr;

    void insert(token incoming_data){
        if (head == nullptr){
            node *new_node = new node;
            new_node->data = incoming_data;
            new_node->next = nullptr;
            head = new_node;
            tail = new_node;
            new_node = nullptr;
        }
        else{
            node *new_node = new node;
            new_node->data = incoming_data;
            tail->next = new_node;
            new_node->next = nullptr;
            tail = new_node;
            new_node = nullptr;
        }
    }
    void print(){
        cerr<<endl;
        if (head == nullptr){
            cerr<<"List is empty";
        }
        else{
            node *active_ptr = head;
            while(active_ptr!=nullptr){
                active_ptr->print();
                cerr<<" --> ";
                active_ptr = (*active_ptr).next;
            }
            cerr<<"NULL\n";
        }
    }
    void del(){
                cerr<<"List before deletion is ";
                print();
                cerr<<"\n";
                cerr<<"Head is at "<<head;
                cerr<<"\n";
        if (head == nullptr){
                    cerr<<"List is empty\n";
            return;
        }
        else if (head->next == nullptr){
            tail = nullptr;
                        cerr<<endl<<"Deleting object at "<<head<<endl;
                        delete head;
            head = nullptr; //keep an eye on this

        }
        else{
            node *temp = head;
            head = temp->next;
                    cerr<<endl<<"Deleting object at "<<temp<<endl;
            delete temp;
            temp = nullptr;
        }
                cerr<<"List after deletion is ";
                print();
        cerr<<"\n";
    }
    bool is_empty(){
        if (head == nullptr){
            return true;
        }
        else return false;
    }

    token first_elem(){
        return head->data;
    }

    ~list(){
        cerr<<"Destructor calling delete"<<endl;
        while (not(head == nullptr)){
            del();
        }
    }
    void insert_front(token incoming_data){
        if (head == nullptr){
            node *new_node = new node;
            new_node->data = incoming_data;
            new_node->next = nullptr;
            head = new_node;
            tail = new_node;
            new_node = nullptr;
        }
        else{
            node *new_node = new node;
            new_node->data = incoming_data;
            new_node->next = head;
            head = new_node;
            new_node = nullptr;
        }
    }

};

class queue{
public:
    list l_queue_list;

    void push(token incoming_data){
        l_queue_list.insert(incoming_data);
    }
    void pop(){
        if(is_empty()){
            return;
        }
        l_queue_list.del();
    }
    token peek(){
        return (l_queue_list.first_elem());
    }
    void print(){
        l_queue_list.print();
    }
    bool is_empty(){
        if(l_queue_list.is_empty())
            return true;
        else return false;
    }
};
class stack{
public:
    list l_stack_list;
    void push(token incoming_data){
        l_stack_list.insert_front(incoming_data);
    }
    void pop(){
        if(is_empty()){
            return;
        }
        l_stack_list.del();
    }
    token peek(){
        return l_stack_list.first_elem();
    }
    void print(){
        l_stack_list.print();
    }
    bool is_empty(){
        if(l_stack_list.is_empty())
            return true;
        else return false;
    }
};

class Thermostat{
public:
    bool heating, cooling, user_setting, valid_command = true; //command is valid until an exception is thrown
    int current_temp, desired_temp;
    Thermostat(string cmd){
        try{
            lexer(&cmd);
            logic();
        }
        catch(...){
            raise_error(7);
        }
    }
private:
    void lexer(string *cmd) {
        string command = *cmd;
        int pos = 0, len = (int)command.length();
        char *start = NULL, *end = NULL;
        if (!(command.find_first_not_of("TtCcHh0123456789+-") == string::npos)){
            raise_error(0);
            return;
        }
        if (command[pos] != 'T' and command[pos] !='t' and !(isdigit(command[pos+1]))){
            raise_error(1);
            return;
        }
        else{
            pos++;
            if(!isdigit(command[pos])){
                raise_error(2);
                return;
            }
            start = &command[pos];
            while(isdigit(command[pos]) and pos<len)
                pos++;
            end = &command[--pos];
            current_temp = parse_digits(start, end);
            pos++;
            if (pos == len){
                user_setting = false;
                return;
            }
            else if(command[pos]!='H' and command[pos]!='h' and command[pos]!='C' and command[pos]!='c'){
                raise_error(3);
                return;
            }
            else{
                user_setting = true;
                if(command[pos] == 'H' or command[pos] == 'h')
                    heating = true;
                if(command[pos] == 'C' or command[pos] == 'c')
                    cooling = true;
                pos++;
                if(!isdigit(command[pos])){
                    raise_error(4);
                    return;
                }
                desired_temp = parse_expression(pos, cmd);
            }
        }
    }
    int parse_digits(char *start, char *end){
        int temperature = 0, place_value = 0;
        for(; end>=start; end--){
            temperature = temperature + (*end-'0') * pow(10,place_value);
            place_value++;
        }
        return temperature;
    }
    queue generate_expression_queue(int pos, string *cmd){ //Generates the expression queue for Shunting-Yard to work on
        string command = *cmd, literal ="";
        queue exp_queue;
        int flag = pos;
        for(; pos<=command.length(); pos++){
            if(command[pos] == '+' or command[pos]=='-'){
                literal = command.substr(flag, (pos-flag)); //Literals are numbers precceded by operators
                token tok1, tok2;
                tok1.data = literal;
                tok2.data =(string(1, command[pos])); //Converting the operator to string-type inline
                tok2.id = "o";
                exp_queue.push(tok1);
                exp_queue.push(tok2);
                flag = pos+1;
            }
        }
        token tok3;
        tok3.data = (command.substr(flag, (command.length()-flag))); //token 3 carries the last digits which aren't succeeded by an operator
        exp_queue.push(tok3);
        return exp_queue;
    }
    queue shunting_yard(queue exp_queue) {  //Simplified execution of shunting-yard because expressions don't have parantheses or operator precedence
        queue output;
        stack ops;
        token temp;
        while(!exp_queue.is_empty()){
            if(exp_queue.peek().id=="n"){
                temp = exp_queue.peek();
                output.push(temp);
                exp_queue.pop();
            }
            else if(exp_queue.peek().id=="o"){
                if(ops.is_empty()){
                    temp = exp_queue.peek();
                    ops.push(temp);
                    exp_queue.pop();
                }
                else {
                    temp = ops.peek();
                    output.push(temp);
                    ops.pop();
                    temp = exp_queue.peek();
                    ops.push(temp);
                    exp_queue.pop();
                }
            }
        }
        while(!ops.is_empty()){
            temp = ops.peek();
            output.push(temp);
            ops.pop();
        }
        return output;
    }
    token eval(token op1, token op2, token operation){// Evaluate binary operation of + or -
        int num1, num2, result;
        token output;
        try {
            num1 = stoi(op1.data);
            num2 = stoi(op2.data);
            if(num1 == 0 or num2 == 0){     // Increment or Decrement by 0 not allowed
                raise_error(6);
                return output;
            }
            if(operation.data == "+"){
                result = num1 + num2;
                output.data = to_string(result);
                return output;
            }
            else if (operation.data == "-"){
                result = num1 - num2;
                output.data = to_string(result);
                return output;
            }
            else{
                raise_error(5);
                return output;
            }
        }
        catch(...){
            raise_error(5);
            return output;
        }
    }

    int reverse_polish_parse(queue exp_queue){ //RPN parsing algorithm
        stack ops_stack;
        token op1, op2, operation, temp;
        while(!exp_queue.is_empty()){
            if(exp_queue.peek().id == "n"){
                temp = exp_queue.peek();
                ops_stack.push(temp);
                exp_queue.pop();
            }
            else if(exp_queue.peek().id == "o"){
                op1 = ops_stack.peek();
                ops_stack.pop();
                op2 = ops_stack.peek();
                ops_stack.pop();
                operation = exp_queue.peek();
                exp_queue.pop();
                token x = eval(op2, op1, operation);
                ops_stack.push(eval(op2,op1,operation));
            }
        }
        try{
            return (stoi(ops_stack.peek().data));
        }
        catch(...){
            raise_error(5);
            return -1000;
        }
    }
    int parse_expression(int pos, string*cmd){//
        queue exp_queue = generate_expression_queue(pos, cmd);
        exp_queue = shunting_yard(exp_queue);
        return reverse_polish_parse(exp_queue);
    }
    void raise_error(int id){
        valid_command = false;

        switch (id) {
            case 0:
                cerr<<"Ilegal characters\n";
                break;
            case 1:
                cerr<<"First letter of command must be T or t\n";
                break;
            case 2:
                cerr<<"T must be followed by current temperature\n";
                break;
            case 3:
                cerr<<"Current temperature must be followed by setting\n";
                break;
            case 4:
                cerr<<"Setting must be followed by a vaid expression\n";
                break;
            case 5:
                cerr<<"Error parsing expression token. Please check\n";
                break;
            case 6:
                cerr<<"Increment or Decrement should be more than 0\n";
                break;
            case 7:
                cerr<<"Please don't walk into a bar and try to order -5 beers. Thanks :) \n";
                break;
            default:
                break;
        }

    }
    void logic(){
        if (heating and current_temp >= desired_temp){
            heating = false;
        }
        if (cooling and current_temp <= desired_temp){
            cooling = false;
        }
    }
};

bool isWellFormedThermostatString(string commands){
    Thermostat thermos(commands);
    return thermos.valid_command;
}

int temperature(string commands){
    Thermostat thermos(commands);
    if (thermos.valid_command){
        return thermos.current_temp;
    }
    else return -1;
}

int setting(string commands){
    Thermostat thermos(commands);
    if (thermos.valid_command and thermos.user_setting){
        return thermos.desired_temp;
    }
    else if(thermos.valid_command){
        return 0;
    }
    else return -1;
}

bool isHeating(string commands){
    Thermostat thermos(commands);
    if (thermos.valid_command)       //Extra security doesn't hurt :)
        return thermos.heating;
    else return false;
}

bool isCooling(string commands){
    Thermostat thermos(commands);
    if (thermos.valid_command)
        return thermos.cooling;
    else return false;
}
int main(){

    string str = "T205h55+5";
    cerr<<"\nThe command is valid: "<<boolalpha<<isWellFormedThermostatString(str)<<endl<<endl;;
    cerr<<"Current temperature is "<<temperature(str)<<endl<<endl;
    cerr<<"User's desired temperature "<<setting(str)<<endl<<endl;
    cerr<<"Heating: "<<boolalpha<<isHeating(str)<<endl<<endl;
    cerr<<"Cooling: "<<boolalpha<<isCooling(str)<<endl<<endl;
}
  • The strange alignment actually makes this much *harder* to read! – BoBTFish Jul 18 '18 at 06:36
  • Sorry, I'll align it normally – ARNAV RAMKRISHNAN Jul 18 '18 at 06:37
  • Thanks. I see nothing immediately wrong with your `delete`, so maybe you do make a mistake building the list in the first place, or at some point take the address of something that isn't actually an allocated node. – BoBTFish Jul 18 '18 at 06:39
  • 1
    1) Did you try to step through your code with a debugger? 2) Please provide [mcve]. 3) Most likely cause for the issues is: undefined behavior, such as: a) double-`delete`ing objects; b) Trying to `delete` uninitialized pointers. 4) It is irrelevant if your "code compiles fine in clang". It compiles fine in g++ as well, since the error you get is a run-time error, not compile-time. – Algirdas Preidžius Jul 18 '18 at 06:39
  • Please provide method of adding node – acade Jul 18 '18 at 06:43
  • I've edited the code into a complete example, including method of addition of nodes – ARNAV RAMKRISHNAN Jul 18 '18 at 06:43
  • 1
    @ARNAVRAMKRISHNAN 1) Is it minimal though? Are those `cout`s are necessary to reproduce the problem? Are all the methods necessary? 2) Is it complete? Can I copy-paste, and run your code, when it doesn't include `main`? – Algirdas Preidžius Jul 18 '18 at 06:45
  • 2
    A wild guess: Rule of Three is violated in this code and the actual program "copies" the list. It results in shallow copy and double-deletion. – AnT stands with Russia Jul 18 '18 at 06:49
  • I've left the cout statements to show that it is not an error involving trying to delete a non-existent object/object not allotted by 'new'. Sorry, I'll add the header and the list and queue classes. However, my main contains yet higher level functions (it was an implementation of Shunting-Yard and RPN parse). Should I add everything? – ARNAV RAMKRISHNAN Jul 18 '18 at 06:50
  • I tried adding a simple `main` inserting 2 elements, compiled with g++ 8.1.0, and had no issues. As @AlgirdasPreidžius mentioned, please carefully read the description of a [mcve]. You should not add everything, you should narrow the code down to the smallest example that exhibits the problem. This way you may even solve the problem yourself, by stripping out all unnecessary confusion. But if you still can't understand the problem at that point, that's fine, but it will be much easier for us to pick up the example and explain what's going wrong in it. – BoBTFish Jul 18 '18 at 06:51
  • You should pass "Token" **as a reference** to insert methods than by value. –  Jul 18 '18 at 06:52
  • @mfromla That is often a recommended good practice (reference-to-`const`, really), but I don't see that it is likely to be relevant to the problem here, so the suggestion will probably just cause more confusion. – BoBTFish Jul 18 '18 at 06:53
  • @BobTFish I was suspecting involvement of copy constructor for temp variable and hence suggested. –  Jul 18 '18 at 06:55
  • @BoBTFish- I've been trying that for a day now, however individual lists, stacks and queues function fine. Even parsing works fine in clang. I wrote the original code in clang, however while copy-pasting it to g++ something breaks down somewhere and I'm not able to reproduce what exactly. I thought the problem is in the list implementation because that's what g++ was telling me – ARNAV RAMKRISHNAN Jul 18 '18 at 06:57
  • @AnT- That was my first guess too. However, I deleted any piece of code that could have been a cause of trouble. At no point there's a = b or a = b where either a or b was allocated by new. Every assignment operator has been applied to pointers and not the actual objects itself – ARNAV RAMKRISHNAN Jul 18 '18 at 07:00

1 Answers1

4

Your list is used as an immediate member of queue and stack classes. Class queue is passed around by value in your code. Since Rule of Three is violated by your list class, it expectedly and unavoidably leads to typical problems, like double deletion, access to deallocated memory and so on.

The current compiler-provided copy constructor and copy assignment operator of list perform shallow-copying of your list objects, i.e. all copies of the same list will refer to the same linked lists of nodes. When one such instance gets destructed, it deallocates the list. After that other copies are left pointing to deallocated memory.

If you really want to pass these classes around by value you have to follow the Rule of Three (or Rule of Five). You have to properly implement copy constructor and copy assignment operator for your list class.

Another solution would be to avoid passing these objects by value. If you decide to go that way, define the copy constructor and copy assignment operator as "deleted". This will make sure you never inadvertently make a shallow copy of these objects.

AnT stands with Russia
  • 312,472
  • 42
  • 525
  • 765
  • Ah thanks! I was applying Rule of Three to the node class and that wasn't helping. I will try applying it to the list class itself – ARNAV RAMKRISHNAN Jul 18 '18 at 07:09
  • However, I'm still intrigued by the cout statements. It clearly prints the memory, then the object at that memory, and the very next line of code tries to delete the object but runs into error. Shouldn't the cout statements behave absurdly while trying to print non-existant object? – ARNAV RAMKRISHNAN Jul 18 '18 at 07:11
  • 1
    @ARNAVRAMKRISHNAN The behavior is undefined. If might behave "absurdly", or it mighf behave "normally". What will happen depends on many unpredictable factors. Deleting an object does not burn a hole in memory. In many cases if leaves the memory content [almost] intact. This is why it might look as if everything is fine, while in reality the list is already "dead". – AnT stands with Russia Jul 18 '18 at 07:22