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

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 ;) .

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

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

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

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:

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.

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)