bug in String::grow...

[thumperj]

I think the line in String::grow:
if (d_reserve > STR_QUICKBUFF_SIZE)
should be:
if (new_size > STR_QUICKBUFF_SIZE)

I've tried all day to test this simple change but I can't verify that this is the fix. For the life of me I can't compile and run with that simple change. My app crashes when trying to init, while loading DLLs (before any code) which tells me that it's a version-itus problem somewhere. And, with the CEGUI lib taking 15 minutes to compile (and since the source is in String.h which is included in everything and causes THE WORLD to recompile), I can't spend any more time on trying right now w/ my pending demo.

So, the basic issue is that concating two strings has an issue when the concat takes the length of the string from below the STR_QUICKBUFF_SIZE threashold to beyond the STR_QUICKBUFF_SIZE.

Hope this helpful.

cheers,
Chris

[CrazyEddie]

The area of code you refer to is not a bug, that code checks to see where the chars should be copied from when growing the string; this is dependant upon the size of the current reserve, when the reserve is > STR_QUICKBUFF_SIZE the data is copied from an allocated buffer, when the reserve is <= STR_QUICKBUFF_SIZE the data is copied from d_quickbuff. The condition al before that code has already done the check on new_size to see if re-allocation is required.

I haven't tested the cause of any problems yet; some minimal code that demonstates the issue would be cool

The long compile time is a pain, this will be addressed in a little while (it's caused by boost, which is going to get ripped out in a few weeks).

CE.

[thumperj]

A simple example:

Code: Select all

    CEGUI::String str = "This is a thirty char string.";
    CEGUI::String another = "This is not.";
    CEGUI::String failed = str + another;

failed will have garbage in it.

Cheers,
Chris

[gcarlton]

I've got no specific help here, but one thing I've noticed is a lack of assertions in cegui. For a complex string class, I think they'd probably help to iron out any bugs.

Obviously there is great support for the proper run-time errors (with proper exception throwing), but a reckon a few CEASSERT(x) around the place would help for ensuring the internal code assumptions are correct. I know I go crazy with them in my code, they're like a safety web just waiting to catch any bugs nice and early.

Also, another good idea is a variant, say CEASSERT_PTR(x), that checks the pointer range is valid. Can also check against x<50 (crops up when offseting a null ptr), and x != cdcdcd etc etc, which in debug catches almost all "obviously" bad pointers.

[thumperj]

I'm with you on that. Especially the "I go crazy with them in my code" part. I use the heck out of asserts. They can be a real time saver. Early and often is the best time to catch bugs, especialy subtle ones. The best bug in the world is the one that explodes like a nuclear bomb so you can't miss it. IMO, CEGUI would do good to have the liberally sprinkled around the code.

cheers,
Chris

[CrazyEddie]

@thumperj:
That code works fine here. Doesn't show up in the debug helper for some reason, but upon manual inspection it's contents are what you'd expect.

@thumperj and gcarlton:
Your observations are acknowledged I think there is not a single assert in the whole library

CE.