2014-03-27 17:56 GMT+01:00 Tavmjong Bah <tavmjong@...8...>:
- 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.
See here: http://www.boost.org/doc/libs/1_55_0b1/libs/utility/operators.htm
- 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).
The iterator would contain a pointer to SPStyle and an integer index into a table like this (note, I'm not 100% sure about the typedef syntax):
typedef SPStyle::*SPIBase SPStyleMember; static SPStyleMember members[] = { &SPStyle::font_size, &SPStyle::font_style, ... NULL };
Incrementing the operator would increase the index, and dereferencing would access the member via the pointer, e.g.
SPIBase const &operator*() const { return _style->*(members[_member_index]); }
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.
There is a way to do this with zero-size mock objects, but this hack would be very elaborate. Converting everything to use getters would be a better long term solution.
- 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.
OK, I see. Another idea is to use offsetof. The object doesn't need to store the pointer to know where its parent is, it can just subtract the offset from the beginning of SPStyle to itself from its 'this' pointer. This technique is used extensively in the Linux kernel.
I am also interested in general feedback... is this the proper way to go?
At first glance, this is definitely a substantial improvement over the existing code.
Regards, Krzysztof