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+.