Can someone help with my seg fault?

This is a discussion on Can someone help with my seg fault? within the C++ Programming forums, part of the General Programming Boards category; My code produces a segmentation fault. I was hoping someone could analyze my code and alert me how to avoid ...

  1. #1
    Registered User
    Join Date
    Sep 2007
    Posts
    119

    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);
    }

  2. #2
    Registered User
    Join Date
    Jan 2005
    Posts
    7,344
    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.

  3. #3
    Registered User
    Join Date
    Sep 2007
    Posts
    119
    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.

  4. #4
    Registered User
    Join Date
    Jan 2005
    Posts
    7,344
    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.

  5. #5
    Registered User
    Join Date
    Sep 2007
    Posts
    119
    I ended up ditching the original implementation and changing it so it works properly. Thanks

  6. #6
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,308
    1. Don't use "this->" everywhere. It only makes the code harder to read. I'd recommend doing a search and replace to remove them all.

    2. Use constructor initialiser lists.

    3. Don't write an empty non-virtual destructor. Unecessary code is bad. YAGNI
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  7. #7
    Registered User
    Join Date
    Jan 2005
    Posts
    7,344
    >> 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.

  8. #8
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Posts
    23,025
    Quote Originally Posted by iMalc View Post
    1. Don't use "this->" everywhere. It only makes the code harder to read. I'd recommend doing a search and replace to remove them all.
    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.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  9. #9
    Captain Crash brewbuck's Avatar
    Join Date
    Mar 2007
    Location
    Portland, OR
    Posts
    7,274
    Quote Originally Posted by Elysia View Post
    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.

  10. #10
    coder
    Join Date
    Feb 2008
    Posts
    127
    Talking about the "this" pointer:
    Code:
    ////constructor
    IntTreeItem::IntTreeItem( int value )
    {
    	this->value = value;
    }
    will the compiler understand the modified following code correctly?
    Code:
    ////constructor
    IntTreeItem::IntTreeItem( int value )
    {
    	value = value;
    }
    Last edited by carlorfeo; 02-29-2008 at 12:27 PM.

  11. #11
    Captain Crash brewbuck's Avatar
    Join Date
    Mar 2007
    Location
    Portland, OR
    Posts
    7,274
    Quote Originally Posted by carlorfeo View Post
    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.

  12. #12
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    22,303
    Speaking of which, that should probably go into an initialisation list instead. How does one initialise a member variable that has the same name as the corresponding constructor parameter?
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  13. #13
    coder
    Join Date
    Feb 2008
    Posts
    127
    Quote 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.

  14. #14
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    22,303
    If we want to initialise the value member variable, we could rename the parameter and write:
    Code:
    IntTreeItem::IntTreeItem(int value_) : value(value_)
    {
    }
    But my question is, if we do not rename the parameter, what should we write for the initialisation list? Is:
    Code:
    IntTreeItem::IntTreeItem(int value) : value(value)
    {
    }
    guaranteed to work?
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  15. #15
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,308
    Quote Originally Posted by laserlight View Post
    But my question is, if we do not rename the parameter, what should we write for the initialisation list? Is:
    Code:
    IntTreeItem::IntTreeItem(int value) : value(value)
    {
    }
    guaranteed to work?
    Yes absolutely! I've been through discussions on that before and been assured that it is fine by the standard.

    My number 1 kinda depended on number 2, which I realised while posting.

    Sorry, you're right his destructors aren't virtual. I wrote that bit after glancing at the code, without looking back at it. Either way, if they aren't needed, don't include them.

    Elysia, you can disagree about using "this->" everywhere. Of course that makes you an obfuscator, and have lost some of the point of OOP, but by all means disagree all you like, you'll be the minority by far.

    There is only one case afaik where you need "this->" apart from disambiguating locals from members (which you can avoid by renaming them), and it relates to some pretty extreme templated code. I've read about it, and I heard the wooshing sound as it flew over my head, and I'd expect to probably never come across it in my lifetime.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

Page 1 of 2 12 LastLast
Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Getting a seg fault
    By ammochck21 in forum C Programming
    Replies: 11
    Last Post: 01-23-2009, 05:27 AM
  2. Seg Fault in Compare Function
    By tytelizgal in forum C Programming
    Replies: 1
    Last Post: 10-25-2008, 04:06 PM
  3. seg fault at vectornew
    By tytelizgal in forum C Programming
    Replies: 2
    Last Post: 10-25-2008, 02:22 PM
  4. weird seg fault
    By Vermelho in forum C Programming
    Replies: 3
    Last Post: 05-10-2008, 09:27 PM
  5. Seg Fault Problem
    By ChazWest in forum C++ Programming
    Replies: 2
    Last Post: 04-18-2002, 04:24 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21