2013/8/1 Jon Cruz <jon@...18...>:
This also looks like a place where the pimpl idiom (private implementation) can be quite helpful. This addresses the issue with 'new', and improves encapsulation. It also happens to fall in line with the major change in direction that was taken for GTK+ 3.x. It moves the members and methods into a private helper class that exists only in the implementation .cpp file and not in the public header .h file.
Please don't. Unless there is a reason to keep a stable ABI, and in the case of internal Inkscape objects there is none, pimpl is just gratuitous obfuscation.
For example, if you want to add a new private function to a pimpl class, you have three choices:
1) add in to the impl object, in which case your functions are in two different objects depending on their access control - if later you want to convert a private function into a public one, you have to rewrite all member accesses. 2) add it on the public object, in which case any gain in compilation speed from avoided header changes is negated. 3) make the public object purely virtual.
Let's say you then want to then add a new public function. If you choise 1) or 2), you can add it and implement it on the public object. If you chose 3), you have to make two functions: a virtual shim on the public object and the real method on the private object.
The pimpl idiom introduces conceptual, coding and runtime overhead and does not provide any significant advantage if there is no stable ABI to keep.
In general, we would want to start a c++ class declaration with a single 'public:' followed optionally by a single 'protected:' section and finally an optional 'private:' section. Also, for a destructor we very often need it to be virtual. Of course, with C-based GTK+ style GObjects we can't really have that... but the pimpl idiom will fix it for us.
The destructor is automatically virtual when the base class destructor is virtual. This will be fixed once Markus's branch is merged.
Then we want to get rid of any function bodies in the header, as that locks us into promising a certain implementation. We want to hide the details inside of the .cpp and not let anyone know what is going on:
If you put the body of a class member function in a header, it is equivalent to putting it in a .cpp file, except the function is automatically marked as inline. Putting the function body in the header does not lock us into anything, since this is an internal Inkscape object and does not have a stable ABI.
By the way, the "SP" of "SPPoint" is something we want to avoid for any new classes. That stands for "SodiPodi" which was the project Inkscape was forked from years ago.
The SP prefix should be kept for the time being, for the sake of consistency. Removing it should be a separate task.
Regards, Krzysztof