CODEWITHSUNDEEP
c++segmentation-faultdestructordelete-operator
Matteo
Matteo

Reputation: 8124

Class Destructor SEGFAULT

In my project I have two classes, the EarleyParser class:

class EarleyParser
{

    public:

        EarleyParser();
        virtual ~EarleyParser();

        void initialize( string filePath, bool probabilityParse );

    private:

        bool probabilityParser;

        typedef unordered_map< string, list<Production> > productionHashTable;
        productionHashTable earlyHashTable;

};

and the Production class:

class Production
{
    public:

        Production();

        Production( float productionProbability, int productionLength, vector< string >* productionContent );

        Production( const Production& copy_me );

        virtual ~Production();

        float getProductionProbability();
        int getProductionLength();
        vector< string >* getProductionContent();

    private:

        float productionProbability;
        int productionLength;
        vector< string >* productionContent;

        void setProductionProbability( float productionProbability );
        void setProductionLength( int productionLength );
        void setProductionContent( vector< string >* productionContent );

};

As you can see above the EarlyParser class has a member element that is an unordered_map, whose key element is a string and value is a list of elements from the Production class.

The code works correctly, and the unordered_map and list get populated, but when calling the standard destructor class of the EarleyParser I get a segmentation fault.

As I understand the default destructor of EarleyParser should call the default destructor of the unordered_map which should call the one of the list which should call for each of its elements the default destructor of the Production class, which is the following:

Production::~Production()
{
    if( this->productionContent != NULL )
        delete this->productionContent; <- line 44
}

Back tracing with Valgrind and GDB didn't give me much help on how to solve the segmentation fault, which is given exactly in the EarleyParser.cpp at the line 44 of destructor.

Should I implement the destructor class, or should the default destructor be ok? Any ideas on what could be causing the segmentation fault?

ADDED COPY CONSTRUCTOR

Production::Production( const Production& copy_me )
{
    if( this->productionContent != NULL )
        this->productionContent = NULL;

    this->setProductionProbability( copy_me.productionProbability );
    this->setProductionLength( copy_me.productionLength );

    this->setProductionContent( copy_me.productionContent );

}

Upvotes: 2

Views: 2206

Answers (4)

Jean Davy
Jean Davy

Reputation: 2240

Any good or bad reason you have for a pointer, use a std::shared_ptr (as member and constructor parameter), the less you do the more you are. std::shared_ptr will make nullptr and deletion for you !

Upvotes: 0

juanchopanza
juanchopanza

Reputation: 227370

There are two options.

  1. Either you allocate a vector dynamically in Production, in which case you need an assignment operator to do a deep copy of the vector pointer. Your copy constructor should do the same. In this case, you should be following the rule of three.

  2. Or, you take a pointer to a vector in the Production constructor and do not perform a deep copy, in which case Production does not own the vector and should not delete it in the destructor.

If you have case 1, I suggest dropping the pointer and holding an std::vector by value.

Upvotes: 1

Kieveli
Kieveli

Reputation: 11075

I'm not seeing any initialization of the productionContent variable. Try initializing it to NULL with initializers. Default values of uninitialized member variables are not null.

What this means is that productionContent != NULL will always be true because of the fact that it starts out being something other than NULL.

Try something like this in all your constructors:

Production::Production( const Production& copy_me ) : productionContent(NULL)
{
...

Upvotes: 1

Tony The Lion
Tony The Lion

Reputation: 63190

Your Rule of Three is incomplete. Since you have a pointer member, you want to make sure have implemented the copy constructor, copy assignment operator and destructor.

Now, because you have a pointer to vector member, I'm going to tell you that you shouldn't have that, but instead just have a std::vector<std::string> or a std::unique_ptr<std::vector<std::string> >.

I don't know why you decided there was a need to hold a pointer to a container, but it's mostly not a good reason, and it's error prone.

You could hold a reference to a container, but you need to make sure it's initialized in the ctor.

The problem with pointers is that they are too easily grabbed for as a "solution" but in reality are extremely error prone and hard to use. If you stopped thinking about pointers and stopped being inclined to use them at every turn, you'd have a much easier time of it.

Upvotes: 4

Related Questions