String handling issues

Jack Claw was in development between 2006-2008 and was released in the Humble Frozenbyte Bundle for the community to play and build upon.
alt_turo
Posts: 195
Joined: Mon Dec 13, 2010 11:06 am

String handling issues

Postby alt_turo » Thu May 26, 2011 2:27 pm

Currently string handling in the code is a mess. Some places use std::string and other places use new[] to allocate char arrays. Lot's of places use string.c_str() when passing parameters and then either copy it explicitly with new[] or implicitly by constructing a new string from it. This leads to slower code, increased memory consumption and high likelihood for bugs. Something Must Be Done. I propose the following:

Use std::string everywhere. Where currently new[] and char * is used, replace with std::string. Where c_str() is used and the result passed as parameter, go change the parameter to std::string.

Strings should be passed by const reference and stored by value. This should avoid most extraneous copies.

However there's this:
http://cpp-next.com/archive/2009/08/wan ... -by-value/
but only if we have C++0x. So in some cases it might be better to pass by value. This leads to the question of supported compilers.

Questions? Comments? Flames?
Turo Lamminen
Alternative Games

CheatCat
Posts: 14
Joined: Sat Apr 16, 2011 10:03 pm

Re: String handling issues

Postby CheatCat » Thu May 26, 2011 2:45 pm

String for the win B) I cannot even see why anyone would use char arrays (even if I do that all the time)

alt_turo
Posts: 195
Joined: Mon Dec 13, 2010 11:06 am

Re: String handling issues

Postby alt_turo » Thu May 26, 2011 2:48 pm

Some of the code, ogui stuff in particular, appears to have been C originally and never properly ported to C++. For storm3d there's the Windows' brain damaged "objects must be deallocated in the same module which allocated them" which makes it pretty much impossible to use std::string across dll boundaries.
Turo Lamminen
Alternative Games

User avatar
Urfoex
Posts: 50
Joined: Fri Apr 15, 2011 11:14 am

Re: String handling issues

Postby Urfoex » Wed Jun 01, 2011 5:59 pm

I'm for std::string.
And also for the new features of C++0x like r-value reference and move and such stuff.
GCC 4.6 should support most of C++0x and MS VS also. (Here is a little list: http://wiki.apache.org/stdcxx/C++0xCompilerSupport)
+-----------------------------------------------------------------\
| Debian testing 64Bit on
| * AMD Phenom x4 905e (4x2500Mhz)
| * 6GB Ram
| * AMD/ATI Radeon HD4770 (fglrx)
+-----------------------------------------------------------------/

dublindan
Posts: 8
Joined: Wed Apr 20, 2011 6:16 am

Re: String handling issues

Postby dublindan » Thu Jun 02, 2011 10:23 am

alt_turo wrote:there's the Windows' brain damaged "objects must be deallocated in the same module which allocated them"

In my opinion, this is good practice even where its not technically required. Object ownership should always be clear and well defined, so this isn't that much of an issue. You then must make sure that no std::string is deallocated in another module. If passing by const reference, or if passing a c-string (eg, to an external library), this isn't an issue.

So I would say use std::string everywhere and pass by const reference (unless you decide on using C++0x).

alt_turo
Posts: 195
Joined: Mon Dec 13, 2010 11:06 am

Re: String handling issues

Postby alt_turo » Fri Jun 03, 2011 1:10 pm

dublindan wrote:In my opinion, this is good practice even where its not technically required. Object ownership should always be clear and well defined, so this isn't that much of an issue.

Yes it should. In practice however this is pretty hard.

You then must make sure that no std::string is deallocated in another module. If passing by const reference, or if passing a c-string (eg, to an external library), this isn't an issue.

Even when passing by const reference you get issues if you then store the string. Since it's just a reference-counted smart container storing it should only increase the refcount. Consider the following:

1. Level loading code tells renderer to load a texture.

2. Renderer loads it and stores the filename into the Texture object. Filename string is not copied but only has its refcount increased.

3. Level loader no longer has need for the filename and discards its own copy. The refcount is decremented and the only instance of the filename string is now in the Texture object.

4. Sometime later the level is unloaded. Game tells renderer to discard the texture.

5. Renderer destroys Texture object. Filename string refcount falls to 0 and so is deallocated.

6. Since the storage for the string (and the refcount) was allocated in main module and deallocation is attempted in renderer module, things blow up.

Since the above works correctly on all Real Operating Systems it's very easy to do it accidentally. And it only blows up on object deallocation which makes the original error very hard to track down.

So I would say use std::string everywhere and pass by const reference (unless you decide on using C++0x).

Agreed. We should however move to monolithic executables to avoid the issue described above. C++0x has the issue that very latest compiler is required. If we do things the optimal way for C++0x then it's suboptimal for earlier versions.

If all string parameters are passed as "const std::string &" then it's relatively simple to change them to pass-by-value in the future. Not so easy the other way.
Turo Lamminen
Alternative Games

dublindan
Posts: 8
Joined: Wed Apr 20, 2011 6:16 am

Re: String handling issues

Postby dublindan » Sun Jun 05, 2011 10:08 pm

In your example, who owns the string?

At first the loader owns it and then it is passed to the renderer, which is in a different module. The loader then is destructed, leaving the renderer as the owner. In large systems I've worked on, this is a no-no.
If another module wants to become the owner, it must make its own copy, so either the owner must outlive all of the users of the object, or the users must create their own copies.

One possible solution which allows the apparent owner to change hands, is to have a resource management subsystem which outlives all other modules (eg, constructed during initialization, destructed on termination) and which is the real owner of resources. This way a resource, in this case a string, is created by the level loader (the memory is allocated and stored by the resource manager - possibly from a memory pool - and a handle or tag is returned to the loader), the filename is then passed to the renderer. The loader is destructed, decrementing the refcount. Some time later the renderer is destructed, again decrementing the refcount. Now that the refcount is zero, the resource manager releases the resource. No problems, as the resource manager is always in the same module.
When I use this method, I pass a reference to the resource manager to all modules, so that resources are always created and destroyed by the resource manager, which lives in the main module. For simple memory management tasks, the resource manager is easy to implement and use. It obviously gets a little harder if it must also manage non-memory resources, but this can be handled by passing callbacks (or functors) for construction and destruction to the resource manager, which would live in the appropriate module. Of course, doing this, you must make sure that the module is not unloaded before the callbacks are no longer needed. Another advantage is that the resource manager allows you to control resource ownership not only between modules, but also between threads.

In code I wrote a few months ago, the resource manager was responsible for more than just holding on to resources - it added an extra layer of abstraction between object references (tags) and the (pooled) memory which actually stored the objects. Most pointers in the code referred to resource managed objects and therefore were replaced by object tags instead. This meant that the resource manager is able to reorder or move memory around (perhaps to defragment a memory pool, or to move short-lived objects into stack allocators, or even to swap objects to disk or persist them in a database - all of these things become theoretically possible with such a scheme) and update the tags and code using managed objects still referred to the correct memory and pointers were not invalidated. Basically, something like what is described in this article.

Of course, implementing such a system retroactively, as would need to be done for Jack Claw, may not be realistic.

Agreed. We should however move to monolithic executables to avoid the issue described above. C++0x has the issue that very latest compiler is required. If we do things the optimal way for C++0x then it's suboptimal for earlier versions.

If all string parameters are passed as "const std::string &" then it's relatively simple to change them to pass-by-value in the future. Not so easy the other way.

A typedef could be used so changing between const reference and pass-by-value requires changing a single line in a header file and recompiling everything. Of course, code would have to be written to always assume that the string is const and not try and modify it, ever.

Code: Select all

typedef const std::string& string;


Return to “Jack Claw Feedback & Development”

Who is online

Users browsing this forum: No registered users and 2 guests