2014-03-27 16:01 GMT+01:00 Tavmjong Bah <tavmjong@...8...>:
Hi all,
I have rewritten most of the style handling code in style.h and style.cpp to use real C++ classes. I would like you all to review the code and give feedback. The code is available at:
lp:~inkscape.dev/inkscape/style
First impressions:
1. The classes do not follow the coding style guidelines. Public methods should be in lowerCamelCase not in snake_case. Members supposed to be private / protected should start with _. http://www.inkscape.org/en/develop/coding-style/
2. If there is operator==, the class should privately derive from boost::equality_comparable<SPStyle> to automatically provide operator!=. There is no need to allow to override it separately. Moreover, some classes seem to overload operator!= with code identical to that present in their base class.
3. std::vector<SPIBase *> properties; - I understand this is supposed to allow easy iteration over style properties. This could be done without any per-instance memory overhead by defining custom iterators that use a static table of member pointers. I can provide a proof of concept implementation. Another approach would be to have a boost::ptr_map or boost::ptr_unordered_map of properties, so that only the properties which are actually present in the style are stored in memory, but I'm not sure how that would perform.
4. The unit-related functions commented with "no change" could be changed to use the functions from util/units.h. (Note that in general I think the recent unit refactoring needs more work, because as it stands now it doesn't solve all the problems and severely impacts performance in certain cases.)
5. Constructor of SPIBase should take Glib::ustring by const reference instead of by value. Members intended to be private should be prefixed with _.
6. Some classes unnecessarily implement an assignment operator which is completely equivalent to the default one.
7. Nitpick: In the SPIString constructor, no_name_string should be anonymous_string, sounds better.
8. The pattern of calling setStylePointer is suboptimal. If these properties always need an SPStyle pointer and cannot be sensibly instantiated without it, they should just take a reference to SPStyle as a construction parameter.
I'll send a second e-mail once I read the code more throrughly
Regards, Krzysztof