Strange typecast switch in sketcher code

[DeepSOIC]

https://github.com/FreeCAD/FreeCAD/blob ... .cpp#L4401

Code: Select all

void free(std::vector<Constraint *> &constrvec)
{
    for (std::vector<Constraint *>::iterator constr=constrvec.begin();
         constr != constrvec.end(); ++constr) {
        if (*constr) {
            switch ((*constr)->getTypeId()) {
                case Equal:
                    delete static_cast<ConstraintEqual *>(*constr);
                    break;
                case Difference:
                    delete static_cast<ConstraintDifference *>(*constr);
                    break;
                case P2PDistance:
                    delete static_cast<ConstraintP2PDistance *>(*constr);
                    break;
                case P2PAngle:
                    delete static_cast<ConstraintP2PAngle *>(*constr);
                    break;
                case P2LDistance:
                    delete static_cast<ConstraintP2LDistance *>(*constr);
                    break;
                case PointOnLine:
                    delete static_cast<ConstraintPointOnLine *>(*constr);
                    break;
                case Parallel:
                    delete static_cast<ConstraintParallel *>(*constr);
                    break;
                case Perpendicular:
                    delete static_cast<ConstraintPerpendicular *>(*constr);
                    break;
                case L2LAngle:
                    delete static_cast<ConstraintL2LAngle *>(*constr);
                    break;
                case MidpointOnLine:
                    delete static_cast<ConstraintMidpointOnLine *>(*constr);
                    break;
                case None:
                default:
                    delete *constr;
            }
        }
    }
    constrvec.clear();
}

Any idea why cast? Seems pretty equivalent to just iterating over constrvec and calling delete it exept for None case. The None case looks funny too, as why wouldn't one want to delete the thing anyway?.. but may be legit.

[eivindkvedalen]

https://github.com/FreeCAD/FreeCAD/blob ... .cpp#L4401

Code: Select all

void free(std::vector<Constraint *> &constrvec)
{
 ...
}

Any idea why cast? Seems pretty equivalent to just iterating over constrvec and calling delete it exept for None case. The None case looks funny too, as why wouldn't one want to delete the thing anyway?.. but may be legit.

Agree, the Constraint class has virtual functions, so the (correct) virtual destructor will be invoked when just doing delete *constr. There is no reason to check for *constr != nullptr; it is ok to delete it anyway. It should be possible to replace the whole loop by just

Code: Select all

std::for_each(begin(constrvec), end(constrvec), std::default_delete<Constraint>());

Eivind

[wmayer]

The None case looks funny too, as why wouldn't one want to delete the thing anyway?.. but may be legit.

For None there is no break so that it falls through to the default case. So, Eivind is right that the whole thing can be replaced with the one-liner.

[DeepSOIC]

ok, thanks