Thread: Can someone help with my seg fault?

My code produces a segmentation fault. I was hoping someone could analyze my code and alert me how to avoid causing it. I have a generic TreeItem, everything extends it in order for a Tree to contain integers, doubles, strings, and Tree's themselves. Thanks

Code:

#include <iostream>
#include <sstream>
#include <string>
#include <typeinfo>

using namespace std;

//Abstract class for entire hierarchy
class TreeItem
{

  public:

   virtual string toString()=0;
   virtual ~TreeItem();

};

//need here because one of the classes uses a node
class Node
{
  private:

   TreeItem *value;
   Node *left;
   Node *right;

  public:

   //Constructors
   Node( TreeItem *value );
   Node();

   //Accessors
   TreeItem* getValue(); //fetch the data

   Node* getLeft();
   Node* getRight();

   //Mutators
   void setRight( Node* right );
   void setLeft( Node* left );

   //Other class functions
   void insert( TreeItem *value );
   double total();
   string toString();
   string print(); //used for tree's

};


//Subclass (A string)
class StringTreeItem : public TreeItem
{
	private:

	 string value;

	public:

	StringTreeItem( string value );
	string toString();
};

//Subclass (A tree)
class ActualTreeItem : public TreeItem
{

  private:

   Node *root;

  public:

   ActualTreeItem();
   double total();
   string toString();
   void insert( TreeItem *value );
};

//Abstract subclass for numeric value hiearchy

//Subclass Hierarchy
class NumericTreeItem : public TreeItem
{
  public:

   virtual ~NumericTreeItem();

};

class FloatTreeItem : public NumericTreeItem
{

  private:

   double value;

  public:

   FloatTreeItem( double value );
   double getValue();
   string toString();

};

class IntTreeItem : public NumericTreeItem
{

  private:

   int value;

  public:

   IntTreeItem( int value );
   int getValue();
   string toString();

};

//End subclass hierarchy

//End Entire Hiearchy


//Class Methods

//TreeItem
TreeItem::~TreeItem(){}

//StringTreeItem
StringTreeItem::StringTreeItem( string value )
{
	this->value = value;
}

string StringTreeItem::toString()
{
	return value; //already a string
}

//ActualTreeItem
//null constructor
ActualTreeItem::ActualTreeItem()
{
	root = NULL;
}

//need to change/fix, for now its just a dummy method
string ActualTreeItem::toString()
{
	string result = "\nempty list";

	if( root != NULL )
	{
		result = root->print();
 	}

 	return result;
}

double ActualTreeItem::total()
{
	double result;

	//need to find the total of all numeric value in current list,
	//ignoring strings and other trees stored as data
	if( root != NULL )
	{
		result = root->total();
	}

	return result;
}

//fix this code
void ActualTreeItem::insert( TreeItem *value )
{
	//Do ordered inserts
	if( root == NULL )
	{
		root = new Node( value );
	}
	else
	{
		root->insert( value ); //recursive insert
	}
}

//NumericTreeItem
NumericTreeItem::~NumericTreeItem(){}

//IntTreeItem

////constructor
IntTreeItem::IntTreeItem( int value )
{
	this->value = value;
}

int IntTreeItem::getValue()
{
	return value;
}

string IntTreeItem::toString()
{
	stringstream stream;

	string retValue;

	//do conversion
	stream << value;
	stream >> retValue;

	return retValue;
}

//FloatTreeItem

//constructor
FloatTreeItem::FloatTreeItem( double value )
{
	this->value = value;
}

double FloatTreeItem::getValue()
{
	return value;
}

string FloatTreeItem::toString()
{
	stringstream stream;

	string retValue;

	//do conversion
	stream << value;
	stream >> retValue;

	return retValue;
}

//Node
//constructor
Node::Node( TreeItem *value )
{
	this->value = value;
	this->left = NULL;
	this->right = NULL;
}

Node::Node()
{
	this->value = NULL;
	this->left = NULL;
	this->right = NULL;

}

TreeItem* Node::getValue()
{
	return value;
}

Node* Node::getLeft()
{
	return left;
}

Node* Node::getRight()
{
	return right;
}

void Node::setLeft( Node* left )
{
	this->left = left;
}

void Node::setRight( Node* right )
{
	this->right = right;
}

void Node::insert( TreeItem *value )
{
	//code here
	if( value != NULL )
	{

    	if( value->toString() < this->toString() )
    	{
			if( this->getLeft() == NULL )
			{
				this->setLeft( new Node( value ) ); //need &?? or is this expression ok?
			}
			else
			{
				this->getLeft()->insert( value );
			}
		}
		else if( value->toString() > this->toString() )
		{
			if( this->getRight() == NULL )
			{
				this->setRight( new Node( value ) );
			}
			else
			{
				this->getRight()->insert( value );
			}
		}

		//if identical, do nothing...no duplicate inserts
	}
}

double Node::total()
{
	double result = 0;

	if( this->getLeft() != NULL )
	{
		result += this->getLeft()->total(); //it gets added
	}

	try
	{
		result += (double)( ( dynamic_cast<IntTreeItem*>( this->getValue() ) )->getValue() ); //cast TreeItem down to IntTreeItem, envoke it's value method, then convert to double
		throw 1;
	}
	catch( int i )
	{
		//do nothing
	}

	try
	{
		result += ( ( dynamic_cast<FloatTreeItem*>( this->getValue() ) )->getValue() ); //cast TreeItem down to FloatTreeItem, envoke it's value method (which gives a double)
		throw 1;
	}
	catch( int i )
	{
		//do nothing
	}


	if( this->getRight() != NULL )
	{
			result += this->getRight()->total(); //it gets added
	}

	return result;
}

string Node::toString()
{
	return this->getValue()->toString();

}

//only called by tree's
string Node::print()
{
	string result = "<";

	//probably need formatting

	if( this->getLeft() != NULL )
	{
		result += "<";
		result += this->getLeft()->print();
	}
	else
	{
		result += "<>";
	}

	result += this->getValue()->toString();

	if( this->getRight() != NULL )
	{
		result += ">";
		result += this->getRight()->print();
	}
	else
	{
		result += "<>";
	}

	return result;
}


//End Class Methods

int main ()
{
	ActualTreeItem BinaryTree1,
				   BinaryTree2;

	BinaryTree1.insert( new IntTreeItem( 10 ) );
	BinaryTree1.insert( new FloatTreeItem( 5.5 ) );
	BinaryTree1.insert( new StringTreeItem( "Ralph" ) );
	BinaryTree1.insert( new StringTreeItem( "Alice" ) );
	BinaryTree1.insert( new IntTreeItem( 20 ) );
	BinaryTree1.insert( new IntTreeItem( 30 ) );
	BinaryTree1.insert( new StringTreeItem( "Ed" ) );

	cout << BinaryTree1.toString();
	cout << "\nTotal: ";
	cout << BinaryTree1.total();

	BinaryTree2.insert( &BinaryTree1 );
	BinaryTree2.insert( new StringTreeItem( "Ralph" ) );
	BinaryTree2.insert( new IntTreeItem( 1 ) );
	BinaryTree2.insert( new StringTreeItem( "Ed" ) );
	BinaryTree2.insert( new FloatTreeItem( 99.5 ) );

	cout << BinaryTree2.toString();
	cout << "\nTotal: ";
	cout << BinaryTree2.total();
	
	delete


	cout << "\n\nEnd processing...\n";

  return (1);
}

Look inside Node::total(). You use dynamic_cast, but don't check the return value. dynamic_cast returns null on failure when casting pointers, so you should be first doing the cast and saving the result, then checking that result against 0 to see if it succeeded. Only if it succeeds should you use that pointer.

Even if you were casting references, in which case dynamic_cast throws an exception when it fails, you aren't catching the bad_cast exception. Is that what the throw 1; lines of code were for? They don't serve any prurpose the way they are now.

I didn't catch this before because I was using my compiler on my windows system. Do I switch the Try/Catch block to catch bad_cast? I'm not familiar with using these in C++, I was hoping by using a Try/Catch block, a failed dynamic cast would stop and execute the catch, and do nothing.

No, dynamic_cast does not throw an exception when it fails to cast a pointer, which is what you are doing. You don't need any kind of try/catch at all there, since there is no code that will throw an exception (except for your code that shouldn't be there).

You need to first do the cast and save the result in a pointer variable, then check that result against 0 to see if it succeeded. Only if it succeeds should you use that pointer. The pointer variable should be the derived type you are casting to.

I ended up ditching the original implementation and changing it so it works properly. Thanks

>> 3. Don't write an empty non-virtual destructor. Unecessary code is bad. YAGNI

Both destructors shown are virtual, although technically the implementation of ~NumericTreeItem is not needed since it must be virtual by virtue of deriving from TreeItem.

Originally Posted by Elysia

I don't know if I would agree with that. There's nothing wrong with it and IMHO, poor indentation hurts much more than this->. And we read poor indented code every day.
Nah. It's a taste thing.

I guess you've never used a language like Python where you are REQUIRED to write this-> (actually, "self.") before every member access. It is awful. It's not a taste thing, it's just ignorant.

Originally Posted by carlorfeo

Talking about the "this" pointer:

Code:

////constructor
IntTreeItem::IntTreeItem( int value )
{
	this->value = value;
}

will the compiler understand he modified following code correctly?

Code:

////constructor
IntTreeItem::IntTreeItem( int value )
{
	value = value;
}

The error in this case was naming a constructor parameter the same as a member variable. Don't do that, and this situation will never occur.

Scopes exist to protect you against ACCIDENTALLY using the wrong binding of a name. They are not provided in order to allow you to wiggle around specifically to name things the same names. That's just bad practice.

Originally Posted by brewbuck

The error in this case was naming a constructor parameter the same as a member variable. Don't do that, and this situation will never occur.

Sure, I understand myself that this may be bad practice, but my question about (usual) compiler's behaviour isn't answered.

Sorry laserlight, due to my english (and c++) knowledge lacks I don't understand what you mean.