Hi Krzysztof,
Thanks for the quick feedback!
On Thu, 2014-03-27 at 16:32 +0100, Krzysztof Kosiński wrote:
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/styleFirst impressions:
- 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/
Easy to do. I just pretty much followed the existing naming in the files, including the preexisting SPIPaint class.
- 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.
You'll need to explain this a bit... I am guessing I can just delete the operator!= from all except the base class as all it does is invert the operator== in all classes.
- 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.
This sounds interesting. I'll need an example. There is already a name look up with a hash table taking place. Using a map in the SPStyle class would eliminate the need for this so it might not be a performance problem (in order to eliminate the long switch).
I thought about only keeping the properties that are actually used but that, I think, requires modifying the way the values are referenced throughout the code base (they are directly accessed). So this might be a long term goal.
- 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.)
This can be a future goal.
- Constructor of SPIBase should take Glib::ustring by const reference
instead of by value. Members intended to be private should be prefixed with _.
Will change the Glib::ustring use. At the moment, almost everything is public due to the way the values are referenced in the rest of the code.
- Some classes unnecessarily implement an assignment operator which
is completely equivalent to the default one.
These can probably be removed now. In the original C code there was a template that required explicit assignment operators (at least the code wouldn't compile without them).
- Nitpick: In the SPIString constructor, no_name_string should be
anonymous_string, sounds better.
OK, I just need to put in something to make debugging easier. All the classes follow the same pattern (e.g. no_name_paint).
- 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 know this is a big ugly but the classes don't always need an SPStyle pointer to be useful. There is at least one instance in the code base where the SPIPaint class (which pre-existed my changes) is used to just read in a a string value.
I'll send a second e-mail once I read the code more throrughly
Looking forward to it.
I am also interested in general feedback... is this the proper way to go?
Tav