3

This code below shows a segmentation fault. However, right when I unquote the cout << endl statement, it gets rid of the seg fault. I also put a print statement without endl, and it hit seg fault right at the start of main(). Can someone please help me figure this out? Thanks!

#include <iostream>

using namespace std;

typedef struct node{
    string city;
    node * next;
} Node;

class Vertex{
    public:
        Vertex(string cityName) {
            x->city = cityName;
            x->next = NULL;
        }

        void printCity() {
            cout << x->city << endl;
        }

    private:
        Node * x;
};

int main() {
    //cout << endl;
    Vertex x("Phoenix");
    x.printCity();

    return 0;
}
Flostin
  • 119
  • 5
aashman
  • 55
  • 7
  • 7
    The data member `x` is not initialized; it's always a dangled pointer. – songyuanyao May 25 '17 at 04:40
  • @songyuanyao What do you mean by that? and how would I fix that? – aashman May 25 '17 at 04:41
  • 1
    Don't use raw pointer if possible, just `Node x;` should be fine. If you have to, add `x = new Node;` in the constructor before using it. (And implement the destructor to `delete` it, and copy/move constructor, assignment operator, ... See [What is The Rule of Three?](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three)) – songyuanyao May 25 '17 at 04:43
  • 1
    As I said, you can just `Node x;`, i.e. don't use pointer at all. Or consider [smart pointers](http://en.cppreference.com/book/intro/smart_pointers). – songyuanyao May 25 '17 at 04:47

3 Answers3

7

You don't appear to be initialising x in your Vertex constructor. This leads to undefined behaviour in dereferencing, so the fact that it only crashes under some circumstances is incidental and irrelevant. You need to fix your undefined behaviour first:

It's not particularly clear why x is a pointer, so consider removing the indirection in its declaration. (*) If using a pointer is intentional, you'll need to allocate some memory to hold the struct and initialise x with it before dereferencing x.

There are also some stylistic issues with this code which can cause you trouble down the line:

  1. You probably want to add the explicit function-specifier to your constructor to avoid accidental implicit conversion from string.
  2. If you keep the pointer field, you'll almost certainly want to replace the automatically generated constructors (default, copy) and implement a destructor. (See the Rule of Three)
pmdj
  • 22,018
  • 3
  • 52
  • 103
  • 2
    This answer could benefit from explaining how to fix it. OP is likely not an expert in C++ and not likely to benefit from what you've said, even if it's technically correct. – Tas May 25 '17 at 04:47
  • @Tas I was in the process of refining it. :-) Feel free to edit my updated answer if you think there's anything missing. – pmdj May 25 '17 at 05:02
1

Three more things

  1. You can also use a unique_ptr instead of a raw pointer to make sure you don't leak memory, change your code to this
  2. Also since you are accepting the string by value, consider moving it into your node class instance (you can also forward it to the constructor of node)
  3. Prefer nullptr to NULL in C++11 and beyond

Chance your code to this

#include <iostream>
#include <memory>

using namespace std;

typedef struct node{
    string city;
    node * next;
} Node;

class Vertex{
    public:
        Vertex(string cityName) : x{std::make_unique<Node>()} {
            x->city = std::move(cityName);
            x->next = nullptr;
        }

        void printCity() {
            cout << x->city << endl;
        }

    private:
        std::unique_ptr<Node> x;
};

int main() {
    //cout << endl;
    Vertex x("Phoenix");
    x.printCity();

    return 0;
}
Curious
  • 20,870
  • 8
  • 61
  • 146
0
#include <iostream>
using namespace std;

typedef struct node{
    string city;
    node * next;
} Node;

class Vertex{
    public:
        Vertex(string cityName) {
            x = new Node();
            x->city = cityName;
            x->next = nullptr;
        }

        void printCity() {
            cout << x->city << endl;
        }
        ~Vertex(){
            delete x;
        }
    private:
        unique_ptr<Node> x;
};

int main() {
    Vertex x("Phoenix");
    x.printCity();

    return 0;
}
Rahim
  • 146
  • 1
  • 8
  • 1
    Please change the type of the pointer to a `unique_ptr` if you can, OP should be informed of the best practices now if possible – Curious May 25 '17 at 06:26