Cannot close file stream

Hi, I've been confused by this error I encountered. I got a segmentation fault when I tried to close a file stream. The funny thing is if I used getline from the stream, it worked. This is the code:

Code:

        include <fstream>
        include <vector>
        include .....

        .......
        using namespace std;

	struct sDemoActInFrame
	{
		int frame; //The frame of the action
		int action; //The action	
		Sint16 cursorX; //Only if action == CURSOR_HAND
		Sint16 cursorY; //Only if action == CURSOR_HAND
		
	};
	
	vector<sDemoActInFrame> mDemoActList;

	fstream filestr; //File stream
	char temp[256]; //Temp buffer for reading
	bool readData=true;

        //Open filestream
	filestr.open ("./Stage_01/demo/demoacts.txt", fstream::in);
	
  	// Begin loading demo action list
  	filestr.getline(temp,256);
	while (readData) 
	{
		while (temp[0] == '#' || temp[0] == '\r' || temp[0] == '\0' || temp[0] == '\n' || strlen(temp) == 0)
		{
			filestr.getline(temp,256);
		}
		if (temp[0]==':') //::end = exit from loop
		{
			readData=false;
		}
		else
		{
			sDemoActInFrame tempDemoAct;
			sscanf ( temp, " %d : %d : %d : %d ", 
				 &(tempDemoAct.frame), &(tempDemoAct.action), &(tempDemoAct.cursorX), &(tempDemoAct.cursorY));
			mDemoActList.push_back(tempDemoAct);
			filestr.getline(temp,256);
		}
		

	}
  	// End loading actions
	filestr.close(); <-- segmentation error

Can anybody tell me what's the problem? This gave me a headache. If this doesn't work, I'll revert it back to C style file operation. Thanks in advance.

BTW, I've changed the code to close the file to:

Code:

  	// End loading actions
	if (filestr.is_open())
		filestr.close();

The is_open() method returned true so I guess the file is still being opened. But why does it returned a segmentation error?

this looks like a problem to me:

Code:

while (temp[0] == '#' || temp[0] == '\r' || temp[0] == '\0' || temp[0] == '\n' || strlen(temp) == 0)
{
     filestr.getline(temp,256);
}

i think should be:

Code:

while (temp[0] == '#' || temp[0] == '\r' || temp[0] == '\0' || temp[0] == '\n' || strlen(temp) != 0)

also, in your 'else' condition, you make a call to getline( )

Code:

else
{
   sDemoActInFrame tempDemoAct;
   sscanf ( temp, " %d : %d : %d : %d ", 
		&(tempDemoAct.frame), &(tempDemoAct.action), &(tempDemoAct.cursorX),  
                &(tempDemoAct.cursorY));
   mDemoActList.push_back(tempDemoAct);
   filestr.getline(temp,256);

which could be subsequently called again in your initial 'if' condition.. without any push_back( ) operations in between.. this will overwrite the value of whatever 'temp' was holding.

Code:

while (temp[0] == '#' || temp[0] == '\r' || temp[0] == '\0' || temp[0] == '\n' || strlen(temp) == 0)
		{
			filestr.getline(temp,256);   // <--Possible subsequent call to getline( ) from a previous 'else' operation in the loop
                }

also, i think this should be called before entering the loop.. instead of repeatedly creating new objects on every 'else' case loop iteration:

Code:

sDemoActInFrame tempDemoAct;      //Create this object before entering the loop and reuse it as necessary

suggestion:
Here is what i think is a better way to make sure you have read to 'end of file'. You used a sentinal char value of ':' to indicate that you have read to end of file.. another way to do this is to test the return value of getline( ):

try this:

Code:

while(filestr.getline(temp,256))
{
  stuff...
  ...
  ...
}

This observations may or may not fix your segmentation problem... post ye' code if you continue to have trouble.

Thanks for the reply. Although what you said make sense, it doesn't fix the problem. I admit that my code is not efficient enough.

Originally Posted by The Brain

this looks like a problem to me:

Code:

while (temp[0] == '#' || temp[0] == '\r' || temp[0] == '\0' || temp[0] == '\n' || strlen(temp) == 0)
{
     filestr.getline(temp,256);
}

i think should be:

Code:

while (temp[0] == '#' || temp[0] == '\r' || temp[0] == '\0' || temp[0] == '\n' || strlen(temp) != 0)

This code is in fact working. What it does was skipping the line with '#' (comment) or '\r' or '\0' or '\n' or if the temp has no chararacters (or equals to '\0'). The last line is only a precaution if the "temp[0] =='\0' " didn't work.

also, in your 'else' condition, you make a call to getline( )

Code:

else
{
   sDemoActInFrame tempDemoAct;
   sscanf ( temp, " %d : %d : %d : %d ", 
		&(tempDemoAct.frame), &(tempDemoAct.action), &(tempDemoAct.cursorX),  
                &(tempDemoAct.cursorY));
   mDemoActList.push_back(tempDemoAct);
   filestr.getline(temp,256);

which could be subsequently called again in your initial 'if' condition.. without any push_back( ) operations in between.. this will erase the original value of whatever 'temp' was holding.

Code:

while (temp[0] == '#' || temp[0] == '\r' || temp[0] == '\0' || temp[0] == '\n' || strlen(temp) == 0)
		{
			filestr.getline(temp,256);   //Possible subsequent call to getline ( ) from a previous 'else' operation in the loop
                }

also, i think this should be called before entering the loop.. instead of repeatedly creating new objects on every 'else' case loop iteration:

Code:

sDemoActInFrame tempDemoAct;      //Create this object before entering the loop and reuse it as necessary

This is only the inefficiency in my code. I admit I code it with the first way I can think of. Again, this code actually works. The getline in the else is needed to get to the next line. It's because I did the iteration this way:

Code:

        filestr.getline(temp,256);
	while (readData) 
	{
              ...
              else
              {
                    ...
                    filestr.getline(temp,256);
               }
        }

The getline in the else is needed because if not, it will always be an infinite loop (because the get pointer will always be the same). And on the second tought it is similar to this:

Code:

 	while (readData) 
	{
             filestr.getline(temp,256);
              ...
              else
              {
                    ...
               }
        }

suggestion:
Here is what i think is a better way to make sure you have read to 'end of file'. You used a sentinal char value of ':' to indicate that you have read to end of file.. another way to do this is to test the return value of getline( ):

try this:

Code:

while(filestr.getline(temp,256))
{
  stuff...
  ...
  ...
}

This observations may or may not fix your segmentation problem... post ye' code if you continue to have trouble.

Although the while with getline as parameter will be a better way, it makes life harder because it makes it harder to skip the get pointer if there's a comment or linefeed.

BTW, the segmentation error still occurs. Even when I've clean-built it.

There are two main concerns I have;

Firstly, you do not check the stream state when reading from it. Which means you will continue reading, even if you reach the end of the stream. You also don't check the return value from sscanf() to determine if you successfully read all required values from the string.

Secondly: what is a Sint16? If the "int" type supplied by your compiler is 32 bit, then the line

Code:

   sscanf ( temp, " %d : %d : %d : %d ", 
		&(tempDemoAct.frame), &(tempDemoAct.action), &(tempDemoAct.cursorX),  
                &(tempDemoAct.cursorY));

will yield undefined behaviour (typically tromping memory, which may happen to be in the stream data structures) if tempDemoAct.cursorX and tempDemoAct.cursorY (which are both of type Sint16) are 16 bit quantities.

It would probably help if you checked if your stream was successfully opened before attempting to read from it.

I'm curious. Can you try changing the exact line grumpy has quoted to this ?

Code:

std::stringstream ss;
ss << &(tempDemoAct.frame) << " : " << &(tempDemoAct.action) << " : "
  << &(tempDemoAct.cursorX) << " : " << &(tempDemoAct.cursorY);
ss >> temp;

>>grumpy

Sint16 is actually a typedef-ed signed short from SDL library (I think). Actually I didn't know the exact type of Sint16, but I guess SDL is the one who will convert this type into the appropriate type. BTW, I think the Sint16 variables were indeed the problem. When I changed it to a simple int, it worked. Thanks for pointing that out.

>>Desolator

Are you sure that's the right code? It didn't work. Nevertheless, I think it won't be a problem as my code is working now. Thanks.

Again, thank you all.

Oh.. Ahah, sorry, I read scanf() instead of sscanf() and even then my post still wouldn't make sense.. Ahah, sorry for that.

Originally Posted by g4j31a5

>>grumpy

Sint16 is actually a typedef-ed signed short from SDL library (I think). Actually I didn't know the exact type of Sint16, but I guess SDL is the one who will convert this type into the appropriate type. BTW, I think the Sint16 variables were indeed the problem. When I changed it to a simple int, it worked. Thanks for pointing that out.
.

Short int should be scanfed with the %hd format