FreeOrion

Here is the somewhat off-topic portion of the CVS Commit Info thread, that is now it's own thread. Let's try to keep this on-topic; it's fairly important.

While I was [making recent changes], I made some notes that everyone should read about problems I found in the existing code:

I removed the redundant DestroyChild() calls in the dtor of FleetWindow. GG::Wnds do this automatically.


I changed this :

to this:


There are several things wrong with the above code:

1) The comment is redundant; you don't need to tell someone that deleting stuff in a destructor is "de-initialization", it's obvious. Also, the term "de-initialize" is awkward.

2) Checking for non-null m_orders does nothing. Deleting a null pointer is a no-op.

3) This is the destructor. No one will ever access m_orders again, since this entire object is being destroyed. So the m_orders = 0 assignment is redundant.


I completely rewrote GalaxySetupWnd. Notable problems with it were:

1) It defined internal enums that mirrored enums elsewhere for galaxy size and shape. This could create hard-to-find version problems between the different enums.

2) Two constants "GALAXY_TYPES" and "TYPE_ROW_HEIGHT" were created as static members of this class. Unless there is a real need to expose these outside of the class (such as to subclasses or the public), these should be local variables in an anonymous namespace in the implementation file.

3) The types of the pointers in the class were not forward declared. This:

became this:


This allows the compiler to skip all the code in the #included files untill it reaches the .cpp file, where they are actually included. This reduces header dependencies and greatly reduces compile time.

4) The XMLEncode() method did not detach the children for which the class maintains explicit pointers. Then the method XMLEncode()ed each such child. But since these children were still attached, they were written to the XML file twice: once as children of GG::Wnd, and once as data members of GalaxySetupWnd.


Commented-out code has been left all over the place. Demo code as well. Please don't leave this stuff lying around for other people to have to read. Use #if 0 ... #endif to remove code that you might want to reactivate later, or for code that only you want to demo for yourself. Better yet, make a local copy for yourself and don't use the repository for scratch paper.


A Universe search predicate IsSystem() was defined locally in a couple of different .cpp files. One of them was called ServerIsSystem(), probably because it conflicted with the IsSystem() declared elsewhere. The way to make something truly local to the .cpp file it is in is to put it in an anonymous namespace:

this is equivalent to:

that is, it allows the definition of a namespace that is accessible only within the current file, since the unique identifier is omitted and the namespace is therefore unavailable by name in other files. Better yet, IsSystem() and other prediacates belong in a separate compilation unit. I have created a Predicates.h file for this purpose. If you add any non-trivial prediactes declare them there and define them in Predicates.cpp (which you'll have to create).


In SystemIcon, there was a GG::Texture pointer that held a heap-allocated texture loaded from file. This means that if there were 100 red dwarfs, there were 100 copies of the red dwarf texture taking up space. GG has a Texture manager to deal with this. SystemIcon now uses GG::App::GetTexture() to get copies of its Textures.


I found a member variable in ServerApp called m_temp. Let's not and say we did, ok?


This was in ClientUniverse.h:

I almost cried. How about not #defining things that could just as easily been declared const int.

ClientUniverse.cpp was every

other

line! Please be careful not to commit files like this. Do a cvs diff on EVERY file before you commit it, so you can write a good description of the changes in the commit, and so you can spot gaffes like this. I do this, even when I'm committing 39 files like I am tonight.

In MapWnd's constructor, there was a line that said something like const ClientUniverse cu = BLAH. This does two things that are morally wrong. One, it copies a huge object for no reason, since it is catching the object in an object and not a reference. Two, when the object cu goes out of scope at the end of the constructor, it is destroyed, just like any automatic object would be at the end of any function. Except that this object contains several hundred pointers to the same number of objects, and it has a destructor that cleans them up when the object is destroyed. So once I populated the Universe, creating a MapWnd in which to view it DESTROYED ITS CONTENTS! I have spent the better part of 2 days trying to find this bug. This is the problem with leaving hastily written demo code in the repository for others to trip over.

I don't want anyone to get excited by this post. I'm a little frustrated, but I'm not trying to attack anyone (names have been omitted to protect the guilty). I'm posting this like this so that maybe people can see what is wrong and fix it, either in their code already committed, or in code they write later.

I happen to know that on some compilers, deleting a null pointer throws a null pointer exception. This may not be true in GCC, but I think we should explicitly test it before writing those out

Also, great idea about writing the predicates as a separate file. This will certainly cut down on code duplication.

And lastly, this post, and probably a lot of Zach's previous don't belong in this thread. It might make sense to create a thread just for the purpose of discussing coding idioms and practices and ways to improve them.

_________________
FreeOrion Programmer

That "delete 0" is a no-op is part of the C++ language standard. If there are compilers out there that barf when they see this, they are very non-compliant, very out-of-date, or both.

I second that opinion. A thread "Coding Style" would be helpful IMHO. Btw, I agree with Zach on most things except on the "replace includes with forward-declarations" part.

I'm not sure why you don't like forward declarations. They are not only a good practice, they are sometimes necessary. I've already come across at least one circular header dependency in our project that was broken by forward declarations.

I was never a fan of forward declarations, mainly because I only want to see classes declared once per project. They can easily lead to confusion. I'll mainly limit my own use of them to where they're necessary (ie. circular dependencies), but thats all. But with DevC++ I'll do anything to speed compile time, and if using forward declarations will do it, then I'll certainly use them more liberally. I usually use the Microsoft compiler, which works much faster than GCC.

Also, to follow up on some previous statements, just an FYI...
The MS C++ 6 compiler is the one that throws a null pointer exception if you delete a null pointer. I believe this may have been fixed in version 7 (which is the .NET version and conforms to the standards much better than any other MS compiler).

_________________
FreeOrion Programmer

It's probably not necessary to say:

if(p)
delete p;

Instead of just:


delete p;

But this little bit of redundancy doesn't hurt anything, especially if one is used to coding on a non-standard compiler.

Regarding forward declarations:

I personally like to have the complete class definition specified in the header so that I dont need to worry about one of those:
"Use of incomplete type" errors in my .cpp file

However, Given the fact that DevC++ compiles like a snail on my system, I think forward declarations are probably better for our purposes.

What concerns me more are the errors and bugs that Zach has mentioned. We did, at one point, way back in the day, come up with a procedure for testing and peer review which I dont think any of us has been following to its fullest. I'll admit that I haven't done thorough unit testing on all of my code (though I did give the serialization for empire a good workout before commiting it). I dont think any of us has ever been doing peer review, and we may want to think about doing that, though it would slow our development down significantly.

JB

_________________
Empire Team Lead

is insufficient because the later using of variable "m_orders" crashes the program even you'll check it with "if" statement.
Simply add new row afrer this:

that allows check it as follow:

or

were NoOrders is empty exception class

Furthermore you can delete it again without checkup

that signifies

and whosoever saying, this is normal practice for most compilers.

Next

is plently excessive, because that's both "#ifndef" and "#endif" directives must be declared inside the header file.

P.S. agree that setting pointer to zero is redundant in destructor (and returning or throwing from destructor is not allowed by C++ standard ).

_________________
This is what you get ...

It *should be* standard practice that every one of your header files should have the following:




That way you NEVER have to worry about whether the header is already defined in your calling class. Just link in the header - if its already been defined then it will all be ignored.

You are both missing the point of this kind of header inclusion guard, like:

#ifndef _GGButton_h_
#include "GGButton.h"
#endif

When you just plain #include a header that is guarded within the header, like:

#include "GGButton.h"

The preprocessor must look at the entire GGButton.h file, since there is an "#if _GGButton_h_" at the beginning that is matched by an "#endif // _GGButton_h_" at the end of the file. To find that #endif it has to read each line of text in between, which may be hundreds or thousands of lines of code.

By doing it the first way above, you eliminate all the work done in reading and preprocessing the file. All this does is decrease your compilation time; it does not reduce cirular dependencies or anything like that. However, the time savings is usually pretty dramatic.

cool, but you may include headers when compiler worries only

_________________
This is what you get ...

Gnu preprocessor implements inclusion guard optimization which makes external inclusion guards unnecessary. I don't know about VC++ or .NET, but I'm under the impression that they've got faster compilation times to begin with.

This only works when you are using GCC, as you note yourself, and only when you are using a fairly recent version of it. If you do the timings yourself, you'll see the improvement I have.

Hi,

I fully agree with using forward declarations for
all classes (...) which aren't used as a member or a
base class.

Naming convention: Why don't we use it? In my opinion
readability would be greatly improved.

http://www.gregleg.com/oldHome/hungarian.html

Ronald.

Hungarian notation is just a pain in the rear, is decreasing the readabilty by focusing on primitive types. A modern design is not working as much with primitive types as with classes and objects.