[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

cleanup patch / design rules

From: David Philippi
Subject: cleanup patch / design rules
Date: Fri, 23 Aug 2002 17:20:37 +0200
User-agent: KMail/1.4.1

Hi all

With sending this mail, I'm going to commit a rather large bunch of changes.
After trying to compile with quite every remotely usefull warning option 
enable, I found warnings about classes having pointer data members but 
neither copy constructor nor operator=   !

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

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.
Therefore, I went through each and every class, declaring a private copy 
constructor and operator= without defining them. Where required I wrote them 
manually even in those cases where the compiler would create correct ones to 
lower the chance that someone will add a pointer member to those classes 
without making a deep copy of it. I fear that I'll probably have overlooked 
some things, especially in classes where already a copy constructor was 
BTW defining only a copy constructor and no operator= is even worse then not 
defining both - a small change in the using code, copy constructor is used 
where operator= was and a bug vanishes mysteriously...

Therefore, I want to add the following design rules:

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.

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).

I'm going to apply the second rule where necessary soon, since I've found a 
few cases where this wasn't done already - without searching.

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] . 
After doing this I'll go on and apply another design rule:

Every subdirectory in the source tree shall be reflected by a name space in 
the source code.

This will require to add a few more subdirectories and getting rid of 
namespace pingus. 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 ...).

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.

Beside those changes which I plan to do in any case (I can't see any sane 
counter argument), I've got a few questions:

- Why is there a init() in every action? Initializing in constructor is much 
- May I make float delta a global constant of some sort?
- 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) ... */.

As with the other changes, a simple approve is enough. I'll do the work 
myself to learn more about Pingus and to walk another step on the long way 
of transforming the code into something I've fun to work with. Right now I 
found huge piles of (probably old) code that simply terrible and screaming 
to be rewritten.

Bye David

PS: While adding copy methods I also rewrote some code to distract myself 
from the boring task at hand. I remember that I rewrote SoundHolder to use a 
map instead of a vector of some self defined pair class and removed some 
useless code from to_string. sstream::freeze has nothing to do with 
releasing the memory, a frozen stream may not be assigned to, that's all.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]