0

I'm new to c++ and I'm facing an odd behavior from std::cout that I don't understand. In particular, when I want to print the value of the second node, using cout << nd.next[2]->next[17]->val, I get some convoluted bytes. However, if I set it to a variable first, e.g string let2 = nd.next[2]->next[17]->val, then use cout << let2, it prints the correct character. My code is below, I was implementing a trie. (Also since I am very new to c++ any other comments about what I am doing wrong in the code is appreciated)

#include <iostream>
#include <set>
#include <iterator>
#include <map>
#include <string>
#include <unordered_map>

using std::set;
using std::cout;
using std::endl;
using std::string;

struct Node {
    Node* next[26]; 
    string val; 

    void insert(string str) {
        cout << "insert " << str << endl;
        insert(str, 0);
    }

    void insert(string str, int idx) {
        if (idx >= str.length()) {
            return;
        }

        char cur = str[idx];
        int curIdx = cur - 'a';
        cout << "cur: " << cur << endl;
        cout << "cur idx: " << curIdx << endl;
        if (!next[curIdx]) {
            Node newNode = Node();
            newNode.val = cur;
            next[curIdx] = &newNode;
        }

        next[curIdx]->insert(str, idx+1);
    }
};

int plus(int a, int b) {
    return a+b;
}
int main() {

    Node nd = Node();
    nd.insert("cryptography");
    string let1 = nd.next[2]->val;
    string let2 = nd.next[2]->next[17]->val;
    cout << "first letter " << let1 << endl; // c
    cout << "second letter " << nd.next[2]->next[17]->val << endl; // wrong
    cout << "second letter " << let2 << endl; // work as expected
    cout << "sum " << plus(1,2) << endl; // work as expected
    // cout << nd.next[2]->next[17]->val << endl;

    return 0;
}
nlzxz
  • 3
  • 1
  • It is not clear what your objective is. You are assigning next[curIdx] to the address of newNode, but on the line after that when you leave the scope, this becomes undefined behavior since the destructor is called for newNode. – user2980207 Dec 12 '19 at 17:12
  • 1
    `Node newNode = Node();` is a locally scoped Automatic variable. `next[curIdx] = &newNode;` stores pointer to locally scoped Automatic variable. Function exits and locally scoped Automatic variable no longer in scope and is destroyed. Using a variable outside its scope is undefined behaviour, and in this case that behaviour allows it to be copied before the memory is reused and turned into gibberish, but not if you directly attempt to print it. Consider using `new` here. – user4581301 Dec 12 '19 at 17:12
  • In general, if you see code that stores a pointer acquired with the `&` operator, examine the code very closely. It's probably wrong. – user4581301 Dec 12 '19 at 17:18

1 Answers1

0

Regarding the second part of your question ("what I am doing wrong"), in the insert() method you create Node() object on stack and assign next[curIdx] with a pointer to this object. But that stack object is destroyed automatically once the execution steps out of the scope where that object is defined, so next[curIdx] ends up pointing to garbage (memory where the object used to be before destroying).
Not sure how the next line is even working, next[curIdx] points to garbage at this point: next[curIdx]->insert(str, idx+1);

Instead you should allocate Node objects on heap with the new operator, ex:

        if (!next[curIdx]) {
            next[curIdx] = new Node();  // allocated on heap
            next[curIdx]->val = cur;
        }

but then you should make sure to deallocate (delete) them at some point to avoid memory leaks. Destructor of Node may be a good place for that – you can recursively delete all non-null Nodes from next array.

Also you could use smart pointers instead of raw pointers, they automatically delete objects when they can't be no longer accessed (garbage collector does that automatically in other languages like Java and C#).

More on stack vs heap: https://www.geeksforgeeks.org/stack-vs-heap-memory-allocation/

AAEM
  • 1,837
  • 2
  • 18
  • 26
  • Also keep an eye out for the [Rule of Three (and friends)](https://en.cppreference.com/w/cpp/language/rule_of_three) if you use the raw [owned](https://stackoverflow.com/questions/49024982/what-is-ownership-of-resources-or-pointers) allocation provided by `new`. If you use smart pointers take care that the `trie` doesn't get so large that it exhausts the program's Automatic storage when it recursively releases all of the `Node`s. Even more on Stack vs Heap. C++ does not guarantee Stacks or Heaps. Prefer to use Automatic and Dynamic. – user4581301 Dec 12 '19 at 17:49
  • Good point, C++ standard uses terms "automatic" and "dynamic" storage duration. Here is a good answer about those terms: https://stackoverflow.com/a/9182244/8070960 – Ivan Novosad Dec 12 '19 at 21:37