Re: cleanup patch / design rules

[Ingo Ruhnke]

David Philippi <address@hidden> writes:
 
> I found warnings about classes having pointer data members but
> neither copy constructor nor operator= !

Most likly because copy c'tor/operator= don't make sense for these
classes, but you are right having them private then is much better than
nothing at all.

> I was also quite shocked to see such warnings stemming from ClanLib.

No, supprise...

> Shallow pointer copies in objects are very dangerous and most often lead to 
> strange, hard to catch bugs. I'd guess that quite a few segfaults were 
> caused by this.

Havn't had a segfault cased by that for years.

> ------------------------------------------------------------------------------------
> Every class shall have either a private declaration of copy constructor and 
> operator= or a working definition. This impiles that a copy constructor 
> calls the copy constructor all base classes and operator= calls operator= of 
> all base classes. If the class is not abstract, operator= shall return *this 
> to allow the objects to be used like integrated types (foo1 = foo2 = foo3).
>
> Copy constructor as well as operator= must assign values to every member of 
> the class.

Thats ok, for most classes that will just mean to have an private copy
c'tor and operator=, since most classes in Pingus aren't copyable.

> Every class containing pointer members shall define a destructor releasing 
> the memory. This implies that every pointer must be initialized in every 
> constructor (be it 0 or a value).

We have quite a few classes that hold reference pointers to other
objects, so just deleting them will not do any good. But we should
probally name these pointers differently or make them via some kind of
smartpointer undeletable or something like that.

> The next thing I'm going to do right now is to apply the rule that every 
> class should reside in it's own .hxx[/.cxx]. 

If classes are small and only a helper class of another one, there is
no need for an extra file, but the class should probally moved into
the other class then.

> This will require to add a few more subdirectories and getting rid of 
> namespace pingus.

Ok.

> I also plan to add some more namespaces whereever I find large
> groups of related classes or where I consider it usefull to improve
> the automatic documentation from doxygen (e.g. I plan to create
> input/buttons, input/pointers ...).

But please don't overuse namespaces, just add them where there really
makes sense and not just but them everywhere just for the fun of it.
 
> Since I'm not yet familar with autoconf/automake/libtool I don't know how to 
> add new subdirs - a small HOWTO would be welcome, else I'll look how the 
> existing subdirs are bound in.

The subdirectories Makefile.in needs to be added to the configure.in
script and to the SUBDIRS variable of the parents directory. After set
run ./autogen.sh.

> - Why is there a init() in every action? Initializing in constructor is much 
>   safer.

In the constructor the action doesn't have a pointer to the Pingu
which it might need for initalisation.

> - May I make float delta a global constant of some sort?

Only in the game-engines main-loop and everything that is called from
there. Everything beside that needs its delta (GUI, Menu's, etc.). The
game_speed variable already holds the needed value.

> - I'd like to restrict doxygen documentation in headers to the short 
>   description and put the detailed description in the .cxx. If the 
>   description is very long it may be best to put it at the end of the .cxx
>   using /** @fn retval class::function (params) ... */.

Ok, even so I don't expect to ever have tons of docu.

> Right now I found huge piles of (probably old) code that simply
> terrible and screaming to be rewritten.

If the part code works for the player, thats enough. After all this is
a game, not some contest to write the perfect C++ code ever.

> sstream::freeze has nothing to do with releasing the memory, a
> frozen stream may not be assigned to, that's all.

#ifdef HAVE_SSTREAM
  std::ostringstream oss;
#else
  std::ostrstream oss;
#endif
  oss << any;
  return oss.str();
  oss << any << std::ends;
  oss.str();

The code under the return looks a bit unreachable.

-- 
WWW:      http://pingus.seul.org/~grumbel/ 
Games:    http://pingus.seul.org/~grumbel/gamedesigns/
JabberID: address@hidden 
ICQ:      59461927