Brief summary of issue / Description of requested feature:
I was looking at some code in the dlgTriggerEditor.cpp file and spotted some very dodgy use of a C-style cast when a C++ dynamic_cast (or even better, a Qt special qobject_cast) is required. For instance consider this:
void dlgTriggerEditor::slot_color_trigger_bg()
{
QTreeWidgetItem* pItem = mpCurrentTriggerItem;
if (!pItem) {
return;
}
int triggerID = pItem->data(0, Qt::UserRole).toInt();
TTrigger* pT = mpHost->getTriggerUnit()->getTrigger(triggerID);
if (!pT) {
return;
}
QPushButton* pB = (QPushButton*)sender();
if (!pB) {
return;
}
int row = ((dlgTriggerPatternEdit*)pB->parent())->mRow;
dlgTriggerPatternEdit* pI = mTriggerPatternEdit[row];
if (!pI) {
return;
}
The cast for sender() seems to be a safety step to ensure that the caller is either an instance of or is derived from a QPushButton so that if it cannot be cast to a QPushButton the conversion fails and pB will be a nullptr - however I do not think it is safe to use a C-style cast for this - see this fine bit of manuals, for the flipping reading of summarising on Stack Exchange: When should static_cast, dynamic_cast, const_cast and reinterpret_cast be used?. The second cast seems equally dodgy but to then leave the open can of petrol next to the unlit candle and the box of matches, the result is dereferenced without a null-check first in order to gain the mRow member value. The correct thing to use normally in both of these situation, is, I believe, a dynamic_cast however Qt helps us in the use of its libraries by providing a less expensive option that can be used on objects that, well, derive from its base qobject class (and include the Q_OBJECT macro to also have the Qt Meta-object functionality), namely the qobject_cast - using that I would rewrite the above as:
void dlgTriggerEditor::slot_color_trigger_bg()
{
QTreeWidgetItem* pItem = mpCurrentTriggerItem;
if (!pItem) {
return;
}
int triggerID = pItem->data(0, Qt::UserRole).toInt();
TTrigger* pT = mpHost->getTriggerUnit()->getTrigger(triggerID);
if (!pT) {
return;
}
QPushButton* pB = qobject_cast<QPushButton*>(sender());
if (!pB) {
return;
}
dlgTriggerPatternEdit* pP = qobject_cast<dlgTriggerPatternEdit*>(pB->parent());
if (!pP) {
return;
}
int row = pP->mRow;
dlgTriggerPatternEdit* pI = mTriggerPatternEdit[row];
if (!pI) {
return;
}
Expected outcome
We should hunt out and destroy fix all C style casts to something more appropriate in our code. Sadly there are three instances in 3rdpart/dblsqd/update_dialog.cpp and several in the C++ part of the 3rdparty/edbee-lib stuff - there are also such casts in the 3rd party/vendor to that - is that 9th party material I wonder‽ - but as they are C code that is perfectly fine.
Extra information, such as Mudlet version, operating system and ideas for how to solve / implement:
A quick and dirty search for the use of C style casts might have once been a global search for ( (space followed by open parentheses) but since we now space for/while/if from their opening ( that is less effective than it once would have been... 🙁
Brief summary of issue / Description of requested feature:
I was looking at some code in the
dlgTriggerEditor.cppfile and spotted some very dodgy use of a C-style cast when a C++dynamic_cast(or even better, a Qt specialqobject_cast) is required. For instance consider this:The cast for
sender()seems to be a safety step to ensure that the caller is either an instance of or is derived from aQPushButtonso that if it cannot be cast to aQPushButtonthe conversion fails andpBwill be anullptr- however I do not think it is safe to use a C-style cast for this - see this fine bit of manuals, for the flipping reading of summarising on Stack Exchange: When should static_cast, dynamic_cast, const_cast and reinterpret_cast be used?. The second cast seems equally dodgy but to then leave the open can of petrol next to the unlit candle and the box of matches, the result is dereferenced without a null-check first in order to gain themRowmember value. The correct thing to use normally in both of these situation, is, I believe, adynamic_casthowever Qt helps us in the use of its libraries by providing a less expensive option that can be used on objects that, well, derive from its baseqobjectclass (and include theQ_OBJECTmacro to also have the Qt Meta-object functionality), namely theqobject_cast- using that I would rewrite the above as:Expected outcome
We should hunt out and
destroyfix all C style casts to something more appropriate in our code. Sadly there are three instances in3rdpart/dblsqd/update_dialog.cppand several in the C++ part of the3rdparty/edbee-libstuff - there are also such casts in the 3rd party/vendor to that - is that 9th party material I wonder‽ - but as they are C code that is perfectly fine.Extra information, such as Mudlet version, operating system and ideas for how to solve / implement:
A quick and dirty search for the use of C style casts might have once been a global search for
((space followed by open parentheses) but since we now spacefor/while/iffrom their opening(that is less effective than it once would have been... 🙁