Hi, as I wrote a few days ago: "If too many of these downcasts appear, it looks to me like a design error. There should probably be a virtual method then." For these situations when a downcast is really necessary, I like the casting macro version best because of its shortness. These macros could of course be replaced by short inline functions as Krzysztof suggested. Regarding the other options: A is best, C is worst :) .

Regards, Markus

-----Ursprüngliche Nachricht----- Von: Johan Engelen [mailto:jbc.engelen@...2592...] Gesendet: Donnerstag, 19. September 2013 22:33 An: Inkscape-Devel Betreff: [Inkscape-devel] Code style question

Hi all, Now with the change to C++ objects (very happy about it!), is it OK to change this

if (SP_IS_LPE_ITEM(item)) { SPLPEItem *lpeitem = SP_LPE_ITEM(item);

to this:

A. if (SPLPEItem *lpeitem = dynamic_cast<SPLPEItem *>(item)) {

I prefer it over the two other options I can think of:

B. SPLPEItem *lpeitem = dynamic_cast<SPLPEItem *>(item); if (lpeitem) {

or

C. if (dynamic_cast<SPLPEItem *>(item)) { SPLPEItem *lpeitem = dynamic_cast<SPLPEItem *>(item);

I like A (more than B or C) because it limits the scope of lpeitem, and avoids duplicating the long dynamic_cast<> thing. Downside is that it makes the if-statement rather long. We use option A in 2geom if I remember correctly.

Please discuss, or add better implementations! I think it is worthwhile to add the outcome to the coding style page for reference.

Thanks, Johan

On Sep 19, 2013, at 1:32 PM, Johan Engelen wrote:

Hi all, Now with the change to C++ objects (very happy about it!), is it OK to change this

if (SP_IS_LPE_ITEM(item))
{
    SPLPEItem *lpeitem = SP_LPE_ITEM(item);

to this:

A. if (SPLPEItem *lpeitem = dynamic_cast<SPLPEItem *>(item)) {

I prefer it over the two other options I can think of:

B. SPLPEItem *lpeitem = dynamic_cast<SPLPEItem *>(item); if (lpeitem) {

or

C. if (dynamic_cast<SPLPEItem *>(item)) { SPLPEItem *lpeitem = dynamic_cast<SPLPEItem *>(item);

Option A is actually a poor coding practice which among other things adds to the chance of bugs. Tools like Coverity will flag assignments within check expressions.

B is probably the closest to better. However you are correct in that limiting scope can be good. The better news is that it is simple to gain that:

{ SPLPEItem *lpeitem = dynamic_cast<SPLPEItem *>(item) if (lpeitem) { ... } }

*However*, the general use of such code should be minimized:

1. It does not follow general C++ and OO design principals. Subclassing, virtual methods, etc. are used

2. The general check was required because of GTK+ being C-based and using their own inheritance approach. This meant storing things as pointers to the generic Widget base class and then converting at point of use. Instead, if one stores and passes the most appropriate parent class, C++ will automatically do the right thing without need of the check and conversion macros

Judicious use of member methods or helper functions can fill in the gaps... but those gaps should not be as prevalent as with C and bare GTK+.