🔒 Locked

is this conversion safe?

Started by Ultra The Vampire Aug 25, 2005 at 8:20 PM 12 replies 2k views

Original Post

Aug 25, 2005 at 8:20 PM

the following code compiles ok ----------------------------------------------------------------------- const irr::c8* PD_Entity_Loader::Get_Entity_Mesh_Path() { ifstream Entity_Definition_File; Entity_Definition_File.open("Entity_List.txt"); char buffer[512]; Entity_Definition_File.getline(buffer, 512); //get the first line in the buffer Entity_Definition_File.close(); irr::c8* temp; Entity_Definition_File >> temp; return (temp); } ----------------------------------------------------------------------- but then i get following warning. Is if the coding safe to use without losing the value of temp and returning it safely or not? if not can someone help me find a different method of converting ASCII text to a const char and returning it safely. Just so you know irr::c8 is a typedef for char. ----------------------------------------------------------------------- c:\coding\irrlicht\ice\ice_3.0\ice_3\src\demo\gamestates\gs_demofakegame.cpp(52) : warning C4700: local variable 'temp' used without having been initialized -----------------------------------------------------------------------

SiCrane

Aug 25, 2005 at 8:24 PM

That code is non-kosher on several levels. You seem to be using the extraction operator to fill a char pointer. Problem 1: you closed the file so extraction isn't going to work. Problem 2: you're filling a character pointer that doesn't have any storage. If that extraction worked, and fortunately it doesn't, you'd probably generate an seg fault as you wrote to random memory. This is bad. Returning char *s from a function in general is bad news. Consider using a std::string instead.

Ultra The Vampire

Aug 25, 2005 at 8:27 PM

problem is, i need to return const char, because of the API i am using, the function i am using this function in requires a const char value to be valid.

Deyja

Aug 25, 2005 at 8:28 PM

Technically streaming into the variable initializes it. However, it's still going explode. First you create a file, read a line from it, then close it. You then proceed to stream from this closed file into a pointer. Assuming you have an overloaded operator>> for that pointer type, it streams into memory who knows where. What's the point of the char array? You never use it. You never created the new entity, but you tried to stream into it anyway.

SiCrane

Aug 25, 2005 at 8:31 PM

Quote:
Original post by Ultra The Vampire
problem is, i need to return const char, because of the API i am using, the function i am using this function in requires a const char value to be valid.

Then make the class you're writing store the std::string as a member variable and return the c_str() in your function.

Ultra The Vampire

Aug 25, 2005 at 8:42 PM

-------------------------------------------------------------------------------
const irr::c8* PD_Entity_Loader::Get_Entity_Mesh_Path()
{
ifstream Entity_Definition_File;
Entity_Definition_File.open("Entity_List.txt");

char buffer[512];
Entity_Definition_File.getline(buffer, 512); //get the first line in the buffer

Entity_Definition_File >> Entity_Path_To_Mesh;

Entity_Definition_File.close();

return (Entity_Path_To_Mesh.c_str());
}
-------------------------------------------------------------------------------

ok, this compiles fine with no warnings or errors.
I should'nt lose data right?

TDragon

Aug 25, 2005 at 8:53 PM

You'll the first line's worth of data that you stored in buffer; of course that may be your intent (but if it is, ignore() is cleaner). The rest of the data from the file will end up in the string you return.

Cheers,
Twilight Dragon

Ultra The Vampire

Aug 26, 2005 at 7:33 AM

i was trying to grab only the first line, and ignore the rest of the file.

SiCrane

Aug 26, 2005 at 7:41 AM

That's not what youn are doing. Try using:

ifstream Entity_Definition_File("Entity_List.txt");std::getline(Entity_Definition_File, Entity_Path_To_Mesh);return Entity_Path_To_Mesh.c_str();

That should get the first line as the file name.

random_thinker

Aug 26, 2005 at 7:59 AM

Ultra,

If you want to, you can add something like these overloaded readline() members to your Input (ifstream extended) class:

void Input::readline(string & str){  std::getline(*this, str);}string Input::readline(){  string str;  readline(str);  return str;}

These should read one line at the time into buffer, so you could do:

Input in(my_filename);in.skiplines(2) // to skip your header.string my_string = in.readline(); // to read one line as string.

Then if you want, you can convert to a char by using my_string.c_str() where you like.

--random

joanusdmentia

Aug 26, 2005 at 8:16 AM

Quote:
Original post by SiCrane
return Entity_Path_To_Mesh.c_str();

Bad SiCrane, bad!! You should NEVER return a pointer returned by std::string::c_str(), but instead use strdup() to make a copy of it and return that pointer.

Quote:
From MSDN
The returned C-style string should not be modified, as this could invalidate the pointer to the string, or deleted, as the string has a limited lifetime and is owned by the class string.

Noting, of course, that the memory will have to be released later by a call to free(), NOT delete.

SiCrane

Aug 26, 2005 at 8:21 AM

Quote:
Original post by joanusdmentia
Bad SiCrane, bad!! You should NEVER return a pointer returned by std::string::c_str(), but instead use strdup() to make a copy of it and return that pointer.

You did notice that the function signature is a const char * right? Not a char * no modification of the buffer. And did you read the thread to notice the small fact this is function is being called by a third party function that won't know to free the strdup()'d pointer?

joanusdmentia

Aug 26, 2005 at 9:14 AM

Quote:
Original post by SiCrane
You did notice that the function signature is a const char * right? Not a char * no modification of the buffer.

Regardless of the returned pointer's constness, as soon as the std::string instance is destroyed so is the returned string. At best, Entity_Path_To_Mesh is a member of the PD_Entity_Loader class and will exist for as long as the class will (and given the class name, it is most likely short lived). This also assumes the string isn't modified elsewhere, for example if he wants to change the definition file and save it (admittadly unlikely given the class name).

I was exaggerating by saying never, however in this instance there is no guarenttee that returning c_str()'s result is safe. While I haven't used Illricht before, I don't see how this could be considered safe.

Quote:
And did you read the thread to notice the small fact this is function is being called by a third party function that won't know to free the strdup()'d pointer?

Good point.

SiCrane

Aug 26, 2005 at 2:43 PM

If you read the thread you'd have noticed that the string is supposed to be a member variable. And if you don't know how the function is being called, then you can't say that its usage is unsafe. In particular, C and C++ idioms for using a function that returns a const char * is to copy or immediately use the returned string. In most cases that means that is perfectly fine if the const char * is the return value of c_str(). const char *s have to point to somewhere that the parent function or object owns or is willing to transfer ownership of and so code that calls such a function will be aware of such a limitation. Think of it this way: c_str() itself is a function that returns a pointer to a buffer that the string owns. Are you going to complain to the standards committee that c_str() is unsafe?

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.