Re: [Inkscape-devel] Cppify Branch Review
Hi there,
SP_TYPE() macros should be reimplemented as dynamic_castso that they generate compile-time errors when something not derived fromSPObject is passed.
I checked that and it is currently impossible withoutchanging a huge amount of code. Some parts use these GSLists and they storevoid-pointers. Thousands of SP_TYPE-lines would fail compilation. I'd suggestdoing this change later.
cppify/src/widgets/rect-toolbar.cpp:117:52: error:cannot dynamic_cast 'items->_GSList::data' (of type 'const gpointer {akavoid* const}') to type 'class SPRect*' (source is not a pointer to class)
Regards, Markus
2013/9/14 Markus Engel <p637777@...1081...>:
Okay. We have to be careful, though, as dynamic_casts add some runtime overhead. After some more refactoring steps we hopefully can omit most of these type checks.
The original GObject implementation calls a function which is equivalent to dynamic_cast, so I think the performance will remain the same.
SP_TYPE() macros should be reimplemented as dynamic_cast so that they generate compile-time errors when something not derived from SPObject is passed.
I checked that and it is currently impossible without changing a huge amount of code. Some parts use these GSLists and they store void-pointers. Thousands of SP_TYPE-lines would fail compilation. I'd suggest doing this change later.
OK, I forgot about Glib-style lists.
Regards, Krzysztof
On 14-9-2013 22:09, Markus Engel wrote:
Hi there,
SP_TYPE() macros should be reimplemented as dynamic_cast so that
they generate compile-time errors when something not derived from SPObject is passed. I checked that and it is currently impossible without changing a huge amount of code. Some parts use these GSLists and they store void-pointers. Thousands of SP_TYPE-lines would fail compilation. I'd suggest doing this change later.
I think it is important that we preserve run-time checking of these casts (from parent to child). Would this be acceptable for now? : #define SP_ITEM(obj) ( SP_IS_ITEM(obj) ? (SPItem*)obj : NULL )
I'm afraid that otherwise, bad casts will be much harder to track down.
Ciao, Johan
I think it is important that we preserve run-time checking of these casts
(from parent to child). Would this be acceptable for now? :
#define SP_ITEM(obj) ( SP_IS_ITEM(obj) ? (SPItem*)obj : NULL )
I'm afraid that otherwise, bad casts will be much harder to track down.
As far as I can see this won't happen, at least not in the existing code. It always looks like this:
void *p .
if (SP_IS_RECT(p)) {
SP_RECT(p)->doWhatever();
}
Every bad cast would have caused an error message on the console.
And for new code, well, be careful with your casts until there's some more refactoring progress. If too many of these to-child-casts appear, it looks to me like a design error. There should probably be a virtual method then.
Regards,
Markus
On 14-9-2013 22:48, Markus Engel wrote:
I think it is important that we preserve run-time checking of these
casts(from parent to child). Would this be acceptable for now? :
#define SP_ITEM(obj) ( SP_IS_ITEM(obj) ? (SPItem*)obj : NULL )
I'm afraid that otherwise, bad casts will be much harder to track down.
As far as I can see this won't happen, at least not in the existing code. It always looks like this:
void *p ...
if (SP_IS_RECT(p)) {
SP_RECT(p)->doWhatever();}
Every bad cast would have caused an error message on the console...
And for new code, well, be careful with your casts until there's some more refactoring progress. If too many of these to-child-casts appear, it looks to me like a design error. There should probably be a virtual method then.
You are right, in all cases we know of, the code does the correct checks :-). But I vaguely remember fixing bugs where the code did not properly check for SP_TYPE. It's just that there might be bad code somewhere, and then a NULL exception is that much easier to kill. The performance hit is not an issue I think, and is something that is easy to measure.
Cheers, Johan
You are right, in all cases we know of, the code does the correct checks
:-).
But I vaguely remember fixing bugs where the code did not properly check
for SP_TYPE.
It's just that there might be bad code somewhere, and then a NULL
exception is that much easier to kill. The performance hit is not an issue I think, and is something that is easy to measure.
I'll add that as you suggested, nevermind ;) .
Regards,
Markus
You are right, in all cases we know of, the code does the correct checks
:-).
But I vaguely remember fixing bugs where the code did not properly check
for SP_TYPE.
It's just that there might be bad code somewhere, and then a NULL
exception is that much easier to kill. The performance hit is not an issue I think, and is something that is easy to measure.
I'll add that as you suggested, nevermind ;) .
Actually, this doesn't work.
SPItem *item=SP_ITEM(desktop->currentLayer()->appendChildRepr(this->repr));
expanded: SPItem *item=((dynamic_cast<const
SPItem*>((SPObject*)desktop->currentLayer()->appendChildRepr(this->repr)) != 0) ? (SPItem*)desktop->currentLayer()->appendChildRepr(this->repr) : 0)
This will call appendChildRepr twice and finally fail in an assertion, because the repr got modified.
Seems like finding a working solution isn't that easy ;) .
Regards,
Markus
2013/9/15 Markus Engel <p637777@...1081...>:
Actually, this doesn’t work.
SPItem *item=SP_ITEM(desktop->currentLayer()->appendChildRepr(this->repr));
expanded: SPItem *item=((dynamic_cast<const SPItem*>((SPObject*)desktop->currentLayer()->appendChildRepr(this->repr)) != 0) ? (SPItem*)desktop->currentLayer()->appendChildRepr(this->repr) : 0)
This will call appendChildRepr twice and finally fail in an assertion, because the repr got modified.
Seems like finding a working solution isn’t that easy ;) .
What Johan's code does is equivalent to:
#define SP_TYPE(x) (dynamic_cast<SPType*>(reinterpret_cast<SPObject*>(x)))
or as a function:
inline SPItem *SP_TYPE(void *x) { return dynamic_cast<SPType*>(reinterpret_cast<SPObject*>(x)); }
There could also be a const variant, but I'm not sure whether it's needed anywhere:
inline SPItem const *SP_TYPE(void const *x) { return dynamic_cast<SPType const*>(reinterpret_cast<SPObject const*>(x)); }
Regards, Krzysztof
On Sun, Sep 15, 2013 at 01:22:04AM +0200, Krzysztof Kosiński wrote:
inline SPItem *SP_TYPE(void *x) { return dynamic_cast<SPType*>(reinterpret_cast<SPObject*>(x)); }
I would just like to point out that void* -> T* does not need a reinterpret_cast, and using such is, in my opinion, confusing. The U *reinterpret_cast<U *>(T *) operator reinterprets the bytes making up an object of type T as an object of type U. Filling in void and SPObject in that sentence means you reinterpret the bytes making up an object of type void as an object of type SPObject, which doesn't make much sense. I suggest using static_cast, instead.
Pippijn
What Johan's code does is equivalent to: #define SP_TYPE(x) (dynamic_cast<SPType*>(reinterpret_cast<SPObject*>(x)))
I replaced the cast macros now and pushed my changes. Instead of this reinterpret_cast I used a c-style cast, so I do not have to care about consts now. There is virtually no const correctness in the existing code, just some single consts that try to make my life hard ;) .
Regards, Markus
I replaced the cast macros now and pushed my changes. Instead of this
reinterpret_cast I used a c-style cast, so I do not have to care about consts now. There is virtually no const correctness in the existing code, just some single consts that try to make my life hard ;) .
I just adjusted factory.h to meet your code style convention, I hope this is okay now. Have you completed your review yet?
Regards, Markus
2013/9/16 Markus Engel <p637777@...1081...>:
I replaced the cast macros now and pushed my changes. Instead of this
reinterpret_cast I used a c-style cast, so I do not have to care about consts now. There is virtually no const correctness in the existing code, just some single consts that try to make my life hard ;) .
I just adjusted factory.h to meet your code style convention, I hope this is okay now. Have you completed your review yet?
I should be able to complete the review in 1-2 days. So far it looks very good.
Regards, Krzysztof
There are basically two issues which stand out:
1. Inconsistent whitespace 2. Use of explicit this-> instead of marking member variables with a leading underscore.
However, I think these two stylistic problems can be fixed later and it's better to commit now than to wait and let this important work get out of sync with trunk.
I committed the merge as revision 12532.
Regards, Krzysztof
On 2013-09-19 04:09 +0200, Krzysztof Kosiński wrote:
There are basically two issues which stand out:
- Inconsistent whitespace
- Use of explicit this-> instead of marking member variables with a
leading underscore.
However, I think these two stylistic problems can be fixed later and it's better to commit now than to wait and let this important work get out of sync with trunk.
I committed the merge as revision 12532.
Copy to clipboard (Ctrl+C) is broken with revision >= 12532:
- Bug #1227827 “Crash on trying to copy opjects from provided SVG file” https://bugs.launchpad.net/inkscape/+bug/1227827
Steps to reproduce: 1) launch current trunk (default prefs, new default doc) 2) draw a rectangle 3) copy it (Ctrl+C) --> crash
Not reproduced with r12531.
On 2013-09-19 04:09 +0200, Krzysztof Kosiński wrote:
There are basically two issues which stand out:
- Inconsistent whitespace
- Use of explicit this-> instead of marking member variables with a
leading underscore.
However, I think these two stylistic problems can be fixed later and it's better to commit now than to wait and let this important work get out of sync with trunk.
I committed the merge as revision 12532.
After the merge in r12532 'File > Document Properties > Grids' no longer display the settings for defined grids (works with r12531, fails with r12532).
Steps to reproduce: 1) launch Inkscape (default prefs, default new doc) 2) toggle on grid (menu 'View > Grid') to create a default grid 3) open 'File > Document Properties > Grids' to edit the settings
Expected result: The settings for the new grid are accessible on the 'Grids' page
Actual result: The 'Defined grids' section is empty.
Warning on the console: ** (inkscape-12532:13355): WARNING **: TypeNotRegistered exception: inkscape:grid
(tested and confirmed on Ubuntu 12.10 and OS X 10.7.5)
participants (5)
-
Johan Engelen -
Krzysztof Kosiński -
Markus Engel -
Pippijn van Steenhoven -
su_v