Sunday, January 5, 2014

A bad workman always blames his tools (Miguel de Icaza inside)

Recently Miguel de Icaza revealed in his blog the fact that they regret the decision to develop Moonlight in C++, you can read about it in here:

http://tirania.org/blog/archive/2014/Jan-04.html

My first thought was, wow he found a way to communicate with a parallel universe where a "Miguel de Icaza" took the decision to got for C and he is now comparing the two projects.

Anyway, he is Miguel de Icaza after all, he is behind the Gnome and Mono projects. Chapeau!
I was curious to look their code base and after having opened some sources here and there I was horrified and I mean it.

Issues I found (just opening a few files not doing a full code inspection and without any static analysis check):

  • CTORs are not using initialization list
  • Arguments of CTORs and functions are not getting parameters as const reference copying basically all the arguments passed
  • Methods that should be marked as const are not(as example KeyTime::HasPercent), this means that const correctness is not used around in the code.
  • classes not meant to be modified after the construction do not have their member marked const
  • Not all classes have all their member initialized
  • Classes have their destructor marked as virtual even when not needed and also if the DTOR is marked as virtual why the CopyCTOR is not implemented or then "disabled"?
  • List reimplemented from scratch and instead to make the List template the List is a standarad bidirectional implementation with Node hosting only the next/prev pointers and a virtual destructor (nice vptr overhead when not needed) then a derived class from Node a template GenericNode. The user of this class has to create his own class Inheriting from GenericNode (see EventObjectNode).
  • brush.cpp:  You accept first a possibile division by zero and then you fix the result
double sx = sw / width;
double sy = sh / height;
if (width == 0)
sx = 1.0;
if (height == 0)
sy = 1.0;

  • collection.cpp the following statement looks suspicious:  

if (n == 0 || n % 1 == 1) {...}

General remarks on the code:
  • I haven't seen a single class not copyable
  • on around 250K line of code just a mere 128 asserts
  • Variables assigned twice without use the first assigned value (see as example CornerRadius::FromStr implementation).
  • Scope variables can be reduced 
  • Pointer cast C-Styled
  • Unused variables

So dear Miguel de Icaza, please fix your code base then we can talk about performances and memory efficiency. Unless your regret has to be read as the following: "We regret to have chosen C++ without having a deep knowledge of it and without any best practice to follow".