0

Singly Linked List and Node classes and the start of the main function, where I wrote a brief outline of the code functionality. The issue is toward the end of the main function. I wrote '...' in place of what I believe to be irrelevant code because it simply parses strings and assigns them to the string temp_hold[3] array.

#include <bits/stdc++.h>
using namespace std;

class Node {
  public:
    string value;
    string attr;
    string tagname;
    Node *next;

    Node(string c_tagname, string c_attr, string c_value) {
        this->attr = c_attr;
        this->value = c_value;
        this->tagname = c_tagname;
        this->next = nullptr;
    }
};

class SinglyLinkedList {
  public:
    Node *head;
    Node *tail;

    SinglyLinkedList() {
        this->head = nullptr;
        this->tail = nullptr;
    }

    void insert_node(string c_tagname, string c_attr,string c_value) {
        Node *node = new Node(c_tagname,c_attr, c_value);

        if (!this->head) {
            this->head = node;
        } else {
            this->tail->next = node;
        }
        this->tail = node;
    }
};

int main(int argc, char **argv) {
    /* storage is a vector holding pointers to the linked lists
       linked lists are created and the linked list iterator sll_itr is incremented when
       previous line begins with '</' and the currentline begins with '<'
       linked lists have nodes, which have strings corresponding to tagname, value, and attribute
    */
    SinglyLinkedList *llist = new SinglyLinkedList();
    vector<SinglyLinkedList*> sllVect;
    sllVect.push_back(llist);
    auto sll_itr = sllVect.begin();

    string temp_hold[3];

    // to determine new sll creation
    bool prev = false;
    bool now = false;


    //input
    int num1, num2;

    cin >> num1; cin >> num2;

    //read input in 
    for (int i = 0; i <= num1; ++i) {
        string line1, test1;
        getline(cin, line1);
        test1 = line1.substr(line1.find("<") + 1);

        //determine to create a new linked list or wait
        if (test1[0] == '/') {  
            prev = now;
            now = true; 
        } else {
            //make a node for the data and add to current linked list
            if (i > 0) {    
                prev = now;
                now = false;

                //if last statement starts with '</' and current statment starts with '<' 
                // then start a new sll and increment pointer to vector<SinglyLinkedList*>
                if (prev && !now) { 
                    SinglyLinkedList *llisttemp = new SinglyLinkedList();
                    sllVect.push_back(llisttemp);
                    sll_itr++;
                }
            }   

            //parse strings from line
            int j = 0;
            vector<string> datastr;
            vector<char> data;
            char test = test1[j];

            while (test) {  
                if (isspace(test) || test == '>') {
                    string temp_for_vect(data.begin(),data.end());
                    if (!temp_for_vect.empty()) {
                        datastr.push_back(temp_for_vect);
                    }
                    data.clear();
                } else
                if (!isalnum(test)) {
                } else {
                    data.push_back(test);
                }
                j++;
                test = test1[j];
            } 

            //each node has 3 strings to fill
            int count = 0;
            for (auto itrs = datastr.begin(); itrs!=datastr.end(); ++itrs) {
                switch (count) {
                  case 0:
                    temp_hold[count]=(*itrs);
                    break;
                  case 1:
                    temp_hold[count]=(*itrs);
                    break;
                  case 2:
                    temp_hold[count]=(*itrs);
                    break;
                  default:
                    break;
                }
                count++;
            }
        }

        cout << "before storing node" << endl;
        (*sll_itr)->insert_node(temp_hold[0], temp_hold[1], temp_hold[2]);

        cout << "after" << endl;
    } 
    cout << "AFTER ELSE" << endl;
    return 0;
}

And here is the line that breaks the code. The auto sll_itr is dereferenced which means *sll_itr is now a SinglyLinkedList* and we can call the insert_node(string, string, string) to add a node to the current linked list. However when I keep the line, anything after the else statement brace does not run, which means the cout<<"AFTER ELSE"<< endl; does not fire. If I remove the insert_node line, then the program runs the cout<<"AFTER ELSE"<< endl; I am unsure what the issue is.

        (*sll_itr)->insert_node(temp_hold[0],temp_hold[1],temp_hold[2]);

        cout << "after" << endl;
    } //NOT HANGING. This closes an else statement.

    cout << "AFTER ELSE" << endl;
    return 0;
} 

Compiled as g++ -o myll mylinkedlist.cpp and then myll.exe < input.txt And input.txt contains

8 3
<tag1 value = "HelloWorld">
<tag2 name = "Name2">
</tag2>
</tag1>
<tag5 name = "Name5">
</tag5>
<tag6 name = "Name6">
</tag6>
chqrlie
  • 131,814
  • 10
  • 121
  • 189
  • Ok. Sorry about that. I will add the missing lines. I just thought it was a bit messy and not relevant. – user10466168 Jan 21 '20 at 20:53
  • If you can reproduce the error without it, by all means leave it out. Unfortunately what you provided cannot reproduce the error you're hunting. Unrelated: Be really careful with `#include `. [Prefer not to do it at all](https://stackoverflow.com/questions/31816095/why-should-i-not-include-bits-stdc-h) because if you include it directly you aren't using it correctly. – user4581301 Jan 21 '20 at 20:56
  • Interesting, I did not know about the `bits/stdc++.h` thank you – user10466168 Jan 21 '20 at 20:59
  • 1
    `sllVect.push_back(llisttemp);` within the very iteration loop controlling `ssl_iter` can, and will, invalidate `ssl_iter` upon internal resize of the vector. – WhozCraig Jan 21 '20 at 21:08
  • `using namespace std;` is also discouraged. Doing that defeats the very purpose for which the std namespace was created. – Eljay Jan 21 '20 at 21:13
  • `for (int i = 0; i <= num1; ++i)` looks like an off-by-one error. i will range from 0 to `num1` for a total of `num1` +1 iterations. You may want `for (int i = 0; i < num1; ++i)` – user4581301 Jan 21 '20 at 21:14
  • `vector` is at its absolute best when you allow it to directly contain data. Pointers to data slow things down with an extra pointer chase and non-contiguous data often wrecks caching. Plus because you're dynamically allocating the linked lists you now have to manage the destruction logic manually. A general rule of thumb in modern C++ is to [avoid using `new`](https://stackoverflow.com/questions/6500313/why-should-c-programmers-minimize-use-of-new). – user4581301 Jan 21 '20 at 21:18
  • ok thank you all for the feedback. I will remember these rules for future coding, and probably redo the structure of this code to make it cleaner. – user10466168 Jan 21 '20 at 21:20
  • Pay special attention to the problem raised by WhozCraig. If it isn't the bug you're hunting it is still a fatal error. The others are mostly nuisances. – user4581301 Jan 21 '20 at 21:31
  • BTW, you only need the `this->` syntax to differentiate member names from parameter names. Less typing without it and less chance to inject defects. – Thomas Matthews Jan 21 '20 at 21:48

1 Answers1

1

Your linked list isn't the problem, at least not the problem here.

A recipe for disaster in the making: retaining, referencing, and potentially manipulating, an iterator on a dynamic collection that potentially invalidates iterators on container-modification. Your code does just that. tossing out all the cruft between:

vector<SinglyLinkedList*> sllVect;
sllVect.push_back(llist);
auto sll_itr = sllVect.begin();

....

SinglyLinkedList *llisttemp = new SinglyLinkedList();
sllVect.push_back(llisttemp); // HERE: INVALIDATES sll_iter on internal resize
sll_itr++; // HERE: NO LONGER GUARANTEED VALID; operator++ CAN INVOKE UB

To address this, you have two choices:

  • Use a container that doesn't invalidate iterators on push_back. There are really only two sequence containers that fit that description: std::forward_list and std::list.
  • Alter your algorithm to reference by index`, not by iterator. I.e. man your loop to iterate until the indexed element reaches end-of-container, then break.

An excellent discussion about containers that do/do-not invalidate pointers and iterators can be found here. It's worth a read.

WhozCraig
  • 65,258
  • 11
  • 75
  • 141
  • Addendum: [A collection of the Iterator Invalidation rules](https://stackoverflow.com/questions/6438086/iterator-invalidation-rules) for the standard library containers. – user4581301 Jan 22 '20 at 21:07