[Adonthell-devel] Rewriting C++ base code

[Pélisson Fabien]

Hi everybody,

My name is fabien pélisson, i am a new member of this mailing list. I
wish to help to the development of adonthell and i have a lot of skills
in C++ programing.
I have recently download the latest CVS archive (modules adonthell-0.3
and adonthell) to firstly take a look at the source code.
So i think there's a lot of things that i can make especially in
rewriting C++ code parts. So if you want me to help, i can manage these
things (i do not have CVS writing access yet).
        
Take a look at the examples below and give me feedback.   


---- Members initialization in constructor --------
actual :
win_theme::win_theme()
{
  normal = NULL;

  mini = NULL;

  ...   
}
           
should be :
win_theme::win_theme()
 : normal(NULL), mini(NULL), ...
{
}

Avoid two initializations of members during the construction process.

---- Deletion with NULL pointer -----
actual:
win_theme::destroy()
{
  if(normal) delete normal;
  if(mini) delete mini;
  ...
}

should be :
win_theme::destroy()
{
 delete normal; normal = NULL;
 delete mini; mini = NULL;
 ...
}

Calling delete with NULL pointer is within the C++ specification and do
not raise any errors.

---- Correct assignment operator construction ----
actual:
win_theme& operator=(win_theme& wt)
{
  destroy();
  ...
  return *this;
}

should be :
win_theme& operator=(const win_theme& wt)
{
        if(this != &wt)
          {
            destroy();
            ...
          }
 return *this;
}

For the const parameter, this is because the right part of an affection
is an rvalue. Moreover, with this form you can make the affectation with
derived class.
The "if" test is actually important for two reasons : 
When you have big classes this avoid to make a copy when the affectation
is made on the same object i.e. a = a;
When the affectation is on the same object i.e. a = a; the result can be
wrong if you release some memory or modify some members during the
operation.


--- Copy constructor with const parameter ----
actual:
win_border::win_border(win_border& wb)
{
  wb_ = NULL;
  ....
  *this = wb;
  refresh();
}

should be:
win_border::win_border(const win_border& wb)
 : wb_(NULL),
   ...
   h_border_template_(new image(*wb.h_border_template_))
   ...
{
  ...
  refresh();
}

Recall that initialization and affectation are 2 differents things.
Within initialization (win_border b; or win_border b(a); or win_border b
= a;) the object is not completly built so you can directly initialize
members directly without calling the affectation operation (that first
destroy and then copy). 

---- Be careful of the operator[] in STL map containers ----
actual:
void win_manager::add_theme(string name)
{
  theme[name] = new win_theme((char*) name.c_str());
}

should be:
void win_manager::add_theme(const string& name) 
{
  win_theme* new_theme = new win_theme(name.c_str());
  hash_map<string, win_theme*>::iterator it = theme.find(name);
  if(it == theme.end())
   {
     theme.insert( pair<string, win_theme*>(name, new_theme));
   }
  else
   {
     delete it->second;
     it->second = new_theme;
   }
}

When the theme name already exist in the map structure the actual
function replace the old win_theme pointer with a new value without
releasing the memory allocated previously.

++ name.c_str() return a const char* i.e. this value can't be affected
to a rvalue. When the name object is destroyed the c_str() data is also
affected.

--------------------------------------------------------------

Fabien.