[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
defined.
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
safer.
- 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.
- cleanup patch / design rules,
David Philippi <=
- Re: cleanup patch / design rules, Ingo Ruhnke, 2002/08/23
- Re: cleanup patch / design rules, David Philippi, 2002/08/24
- Re: cleanup patch / design rules, Ingo Ruhnke, 2002/08/24
- Re: cleanup patch / design rules, Kenneth Gangstoe, 2002/08/24
- Re: cleanup patch / design rules, Kenneth Gangstoe, 2002/08/24
- Re: cleanup patch / design rules, Ingo Ruhnke, 2002/08/24
- Re: cleanup patch / design rules, Kenneth Gangstoe, 2002/08/24
- Re: cleanup patch / design rules, David Philippi, 2002/08/24
- Re: cleanup patch / design rules, Ingo Ruhnke, 2002/08/24
- Re: cleanup patch / design rules, David Philippi, 2002/08/25
- Prev by Date:
Re: none
- Next by Date:
Pingus -bugs?- under windows...
- Previous by thread:
Re: [Pingus-CVS] CVS: Games/Pingus/data/sounds chink.wav,1.1,1.2 digger.wav,1.1,1.2 goodidea.wav,1.1,1.2 ohno.wav,1.1,1.2 plop.wav,1.1,1.2 splash.wav,1.1,1.2 tick.wav,1.1,1.2 ting.wav,1.1,1.2 yipee.wav,1.1,1.2
- Next by thread:
Re: cleanup patch / design rules
- Index(es):