Patch for /trunk/include/Game/Game.h

[GoogleCodeExporter]

Fix segfault on Linux that was worked around by STDCPP_LINUX_HACK.
Mem_Zero on a (struct containing a) std::string (or custom String object) is 
not a good idea and will probably leak memory, even if it works.

(BTW: Using your own string and list classes is usually a bad idea, 
std::string, std::vector etc are proven to work..)

Original issue reported on code.google.com by metalcae...@gmail.com on 24 Sep 2011 at 8:08

Attachments:

• Game.h.patch

[GoogleCodeExporter]

Goes down next commit I do.

Thanks.

Original comment by forsaken...@gmail.com on 24 Sep 2011 at 10:23

[GoogleCodeExporter]

After taking a quick look at uses of Mem_Zero, I guess that it may cause more 
problems.
Even if it doesn't cause crashes (haven't seen a String or some other more 
complex type yet in one of the overwritten structs, but haven't checked all of 
them), It most probably causes memory-leaks, because the structs contained 
pointers.. if they are nulled, the data they point to may leak.
(But maybe it's just data on the stack or there are still other pointers, this 
needs more investigation.)

Cheers,
- Daniel

Original comment by metalcae...@gmail.com on 24 Sep 2011 at 10:45

[GoogleCodeExporter]

Any Mem_Zeros on structs containing strings were a mistake brought over from 
when we switched from C-style strings to mine; if you can find any, let me know.

Yes, std::string and std::vector are proven to work, but they are annoying to 
work with at times and the API is not entirely clear in all cases - I created 
custom classes which appear more user-friendly and adhere to our code 
standards. They are simplistic templates, and have been tested in nearly every 
use scenario to work just fine.

Any mem_zeros on structs, or at least ones that I've consciously written, 
should have had all of their memory freed beforehand (keep in mind we use a 
tagged memory system, and just free the entire tag, so we don't have to deal 
with re-initialization and all that crap).

-P

Original comment by Jonno.5000 on 24 Sep 2011 at 11:13

[GoogleCodeExporter]

Original comment by Jonno.5000 on 24 Sep 2011 at 11:26

• Changed state: Verified

[GoogleCodeExporter]

Your custom String, List and Exception Classes don't even build on Linux/GCC 
because of circular dependencies (Strings throw Exceptions and an Exception 
gets a String as argument..).

I'm currently playing with a fixed r520. The Vector class didn't compile 
either, but that was easy to fix, I may post a patch for that soon.

However, it seems like save games don't work (the game crashes on load) - 
probably because SIntermissionState is just written as a blob into the savegame 
and restored as such and std::string probably does not like this. I haven't 
looked too close at your own String class, but I'd be surprised if it works - 
after all it stores a pointer to heap allocated data (_array) containing the 
string itself and this data will not be serialized when just dumping the 
SIntermissionState struct as a blob (even if the data is saved somewhere else 
the address won't be the same after loading the savegame, I guess).

Cheers,
- Daniel

Original comment by metalcae...@gmail.com on 24 Sep 2011 at 11:31

[GoogleCodeExporter]

Last time I checked, gcc built it just fine. Though, that was before the 
"String" class was introduced... You're welcome if you want to do something 
about that, as I'm pretty much done with linux.

Also, try making a small program where you write a class' contents into an ad 
hoc file, and one of the class' contents is in fact a C string. See what 
happens when you load it back. If the results are what you're expecting (char* 
being written address instead of the contents) go ahead and I/we'll look into 
it.

Original comment by forsaken...@gmail.com on 25 Sep 2011 at 2:05

[GoogleCodeExporter]

I've done this, using simplified versions of your Read()/Write() functions.
Note the segfault when trying to read the serialized file again :) (If it 
doesn't segfault for whatever reason, it will at least not contain "What's 
gonna happen?" but "muhahaha".)

You can also look at the file (bla.dat) in a hex editor: You won't find the 
string in it.

Also: sizeof(Foo) doesn't change (of course not, it's a compile-time constant), 
no matter how long the string pointed to by str is, so Write(buffer, 
sizeof(TType)); etc can't possibly write f.str.

So to write structs that contain pointers to files, you need proper 
serialization..

In fact, one reason for me to look at ccq2 was that proper serialization (and 
thus more portable savegames) are a lot easier with object oriented code.. 

Original comment by metalcae...@gmail.com on 25 Sep 2011 at 7:15

Attachments:

• serialize.cpp

[GoogleCodeExporter]

Exceptions throwing String was replaced in my copy, and now only accepts const 
char* (this was done a long time ago, I just haven't commited it yet).

Again, as for std::string saving, it is completely unintended and I did not 
write that consciously - basically what happened was I was focused more on 
entities saving, and somewhere near the middle of the process I had changed 
most of the fixed-length strings to std::string and hadn't really thought about 
the consequences. I am completely aware of serialization, and before you go 
yelling at me and going Y U NO SERIALIZE STD STRINGS DON'T FWRITE, look at 99% 
of the rest of the game where I DO save everything properly.

It was an oversight, and nothing more.

-P

Original comment by Jonno.5000 on 25 Sep 2011 at 11:35

[GoogleCodeExporter]

Also, if you want to be added to the project members list so you can commit 
properly, just let me know - I have no problem with that. I just don't have 
time lately with school to work on this as much as I could before, but I would 
like to get it to a "working" state once again :)

-P

Original comment by Jonno.5000 on 25 Sep 2011 at 11:43

[GoogleCodeExporter]

Original comment by Jonno.5000 on 26 Sep 2011 at 12:11

• Changed state: Started

[GoogleCodeExporter]

Am 26.09.2011 01:35, schrieb cleancodequake2@googlecode.com:

Ok, I may give the latest revision another try, but I still think using 
std::string and std::vector would be a better idea than NIH-ish custom 
classes. (I've had pretty bad experiences with custom String and List 
classes in the past..)

Relax, my answer was referring to Forsaken's reply:
"Also, try making a small program where you write a class' contents into 
an ad hoc file, and one of the class' contents is in fact a C string. 
See what happens when you load it back. If the results are what you're 
expecting (char* being written address instead of the contents) go ahead 
and I/we'll look into it."

I just did what he suggested and explained, why it can't work.

Original comment by metalcae...@gmail.com on 26 Sep 2011 at 9:17

[GoogleCodeExporter]

WTF, the quoted text from comment 8 was removed..

my first paragraph was after your first paragraph (the one about strings) and 
my second paragraph about your second paragraph (the one about serialization).

Cheers,
- Daniel

Original comment by metalcae...@gmail.com on 26 Sep 2011 at 9:21

[GoogleCodeExporter]

Yeah I know, I just want to make sure you know that I'm not incompetent :p 
Wasn't meant to be aggressive.

I know a lot of people have bad experiences with custom classes, but C++ gives 
us the tools to do this, and I felt it was a lot simpler to use our own than 
try to adapt and mix styles with our style and STL's style, which are 
completely different. Most large libraries tend to use new versions to make 
positive that they are cross platform - although that's not necessarily the 
case here, it does show that there are reasons to do so.

I have a few rookie programmers who I often have look at the code to see if 
there is anything that "sticks out" to them, and back when we used STL they 
wondered why they looked so out of place, so I figured that it'd be better if 
they mixed better with the style I tried to go with.

Would you like to be a project member?

-P

Original comment by Jonno.5000 on 26 Sep 2011 at 9:52