On Apr 28, 2013, at 11:56 AM, Markus Engel wrote:
Hi there, I removed those c++11 parts now and went on with c++ifying the SPEventContexts. These were even easier to convert, I should have started with them… As far as I can tell, Inkscape did not get any slower and memory consumption even decreased a bit. At least that’s what /usr/bin/time -v inkscape -z share/examples/car.svgz -e car.png says.
Btw, I found a little mistake in the source code: src/widgets/sp-attribute-widget.cpp:262:15: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand] if (flags && SP_OBJECT_MODIFIED_FLAG) ^ ~~~~~~~~~~~~~~~~~~~~~~~ src/widgets/sp-attribute-widget.cpp:262:15: note: use '&' for a bitwise operation if (flags && SP_OBJECT_MODIFIED_FLAG) ^~ & src/widgets/sp-attribute-widget.cpp:262:15: note: remove constant to silence this warning if (flags && SP_OBJECT_MODIFIED_FLAG) ~^~~~~~~~~~~~~~~~~~~~~~~~~~
Maybe one of you can correct that as I don’t have write access…
Thanks. I'm looking into that now. Good catch.
I did a quick find/grep for a rough pattern and spotted a few more of those.
Whenever we hit such a problem, however, we do have to follow-up on the implications of a fix. Since we flipped some logic, it's possible we can uncover some bad behavior, or some behavior that was dependent on the broken logic. So just skim to look at such when we find something like this.
Of course following up on that I found this bit of code:
/* TODO: what is this??? case GDK_O: if (MOD__CTRL && MOD__SHIFT) {
That looks like the same type of problem... however it turns out we have a few evil things left in macros.h
First we should not leave macros in C++ code, but more importantly we need to check the quality of the macros themselves: #define MOD__SHIFT (event->key.state & GDK_SHIFT_MASK)
That's not a good macro even for C code. The key is that it does not take in a parameter, so one has to know the proper name and always use the same name everywhere in the code. Not good at all.
A C cleanup could be #define MOD__SHIFT(event) ((event)->key.state & GDK_SHIFT_MASK)
But since we don't want macros at all, we can use a different approach. A non-friend, non-member helper function can work
bool isModSetShift(GdkEvent const *event) { return event->key.state & GDK_SHIFT_MASK; }
...
Anyway, first I'll need to fix those macros to at least take a param. Then I'll be able to see and fix the logic errors. Then we can look at getting rid of those macros. (Normally the compiler will optimize such helper functions, regardless of using or omitting the 'inline' keyword. We'll just need to look at the impact)