ronantakizawa/codereview-bench huggingface dataset:
Repository: FreeCAD/FreeCAD
File: src/Mod/Sketcher/App/planegcs/Constraints.h (C/C++)

Code before:
        if (grad) {
            *grad = 0.;
        }
    };
    virtual double error()
    {
        double err;
        errorgrad(&err, nullptr, nullptr);
        return scale * err;
    };
    virtual double grad(double* param)
    {
        if (findParamInPvec(param) == -1) {
            return 0.0;
        }

        double deriv;
        errorgrad(nullptr, &deriv, param);

        return deriv * scale;
    };
    // virtual void grad(MAP_pD_D &deriv);  --> TODO: vectorized grad version
    virtual double maxStep(MAP_pD_D& dir, double lim = 1.);

    // Evaluates the value of the constraint and assigns it to
    // the value parameter, called on driving constraints to
    // find the parameter of interest without solving
    // Note: not implemented for constraints which do not have a value
    virtual void evaluate()
    {}

    // Finds first occurrence of param in pvec. This is useful to test if a constraint depends
    // on the parameter (it may not actually depend on it, e.g. angle-via-point doesn't depend
    // on ellipse's b (radmin), but b will be included within the constraint anyway.
    // Returns -1 if not found.
    int findParamInPvec(double* param);
};

// Equal
class ConstraintEqual: public Constraint
{
private:
    double ratio;
    double* param1()
    {
        return pvec[0];
    }
    double* param2()
    {
        return pvec[1];
    }

Review comment (suggestion):
```suggestion
    // the value parameter, called on driven constraints to
```

Code after:
        if (grad) {
            *grad = 0.;
        }
    };
    virtual double error()
    {
        double err;
        errorgrad(&err, nullptr, nullptr);
        return scale * err;
    };
    virtual double grad(double* param)
    {
        if (findParamInPvec(param) == -1) {
            return 0.0;
        }

        double deriv;
        errorgrad(nullptr, &deriv, param);

        return deriv * scale;
    };
    // virtual void grad(MAP_pD_D &deriv);  --> TODO: vectorized grad version
    virtual double maxStep(MAP_pD_D& dir, double lim = 1.);

    // Evaluates the value of the constraint and assigns it to
    // the value parameter, called on driven constraints to
    // find the parameter of interest without solving
    // Note: not implemented for constraints which do not have a value
    virtual void evaluate()
    {}

    // Finds first occurrence of param in pvec. This is useful to test if a constraint depends
    // on the parameter (it may not actually depend on it, e.g. angle-via-point doesn't depend
    // on ellipse's b (radmin), but b will be included within the constraint anyway.
    // Returns -1 if not found.
    int findParamInPvec(double* param);
};

// Equal
class ConstraintEqual: public Constraint
{
private:
    double ratio;
    double* param1()
    {
        return pvec[0];
    }
    double* param2()
    {
        return pvec[1];
    }
Repository: FreeCAD/FreeCAD
File: src/Mod/Sketcher/App/planegcs/Constraints.cpp (C++)

Code before:
            if (param == bigradius) {
                drad = 1.0;
            }
            else if (param == smallradius) {
                drad = -1.0;
            }
            else if (param == distance()) {
                drad = (*distance() < 0.) ? 1.0 : -1.0;
            }
            if (length_ct12 > 1e-13) {
                *grad = -dlength_ct12 + drad;
            }
            else {  // concentric case
                *grad = drad;
            }
        }
    }
}
void ConstraintC2CDistance::evaluate()
{
    double dx = *c1.center.x - *c2.center.x;
    double dy = *c1.center.y - *c2.center.y;
    double cdist = std::sqrt(dx * dx + dy * dy);

    double bigradius = (*c1.rad >= *c2.rad) ? *c1.rad : *c2.rad;
    double smallradius = (*c1.rad >= *c2.rad) ? *c2.rad : *c1.rad;

    if (cdist > bigradius && cdist > smallradius) {
        *distance() = cdist - bigradius - smallradius;
    }
    else {
        *distance() = bigradius - smallradius - cdist;
    }
}

// --------------------------------------------------------
// ConstraintC2LDistance
ConstraintC2LDistance::ConstraintC2LDistance(Circle& c, Line& l, double* d)
    : circle(c)
    , line(l)
{
    pvec.push_back(d);
    this->circle.PushOwnParams(pvec);
    this->line.PushOwnParams(pvec);

    origpvec = pvec;
    pvecChangedFlag = true;
    rescale();
}

ConstraintType ConstraintC2LDistance::getTypeId()

Review comment (suggestion):
```c++
#include <algorithm>
```

```suggestion
    auto [smallradius, bigradius] = std::minmax(*c1.rad, *c2.rad);
```

Code after:
            if (param == bigradius) {
                drad = 1.0;
            }
            else if (param == smallradius) {
                drad = -1.0;
            }
            else if (param == distance()) {
                drad = (*distance() < 0.) ? 1.0 : -1.0;
            }
            if (length_ct12 > 1e-13) {
                *grad = -dlength_ct12 + drad;
            }
            else {  // concentric case
                *grad = drad;
            }
        }
    }
}
void ConstraintC2CDistance::evaluate()
{
    double dx = *c1.center.x - *c2.center.x;
    double dy = *c1.center.y - *c2.center.y;
    double cdist = std::sqrt(dx * dx + dy * dy);

    auto [smallradius, bigradius] = std::minmax(*c1.rad, *c2.rad);

    if (cdist > bigradius && cdist > smallradius) {
        *distance() = cdist - bigradius - smallradius;
    }
    else {
        *distance() = bigradius - smallradius - cdist;
    }
}

// --------------------------------------------------------
// ConstraintC2LDistance
ConstraintC2LDistance::ConstraintC2LDistance(Circle& c, Line& l, double* d)
    : circle(c)
    , line(l)
{
    pvec.push_back(d);
    this->circle.PushOwnParams(pvec);
    this->line.PushOwnParams(pvec);

    origpvec = pvec;
    pvecChangedFlag = true;
    rescale();
}

ConstraintType ConstraintC2LDistance::getTypeId()
{
Repository: FreeCAD/FreeCAD
File: src/Mod/Sketcher/App/planegcs/Constraints.cpp (C++)

Code before:
    else if (grad) {
        if (param == distance()) {
            *grad = 1.0;
            if (length < *circle.rad) {
                *grad = -1.0;
            }
        }
        else if (param == circle.rad) {
            *grad = 1.0;
        }
        else {
            *grad = -dlength;
        }
    }
}
void ConstraintP2CDistance::evaluate()
{
    double h, dh;
    h = value(dh, nullptr);

    if (h < *circle.rad) {
        *distance() = *circle.rad - h;
    }
    else {
        *distance() = h - *circle.rad;
    }
}

// --------------------------------------------------------
// ConstraintArcLength
ConstraintArcLength::ConstraintArcLength(Arc& a, double* d)
    : arc(a)
{
    pvec.push_back(d);
    this->arc.PushOwnParams(pvec);

    origpvec = pvec;
    pvecChangedFlag = true;
    rescale();
}

void ConstraintArcLength::ReconstructGeomPointers()
{
    int i = 0;
    i++;  // skip the first parameter as there is the inline function distance for it
    arc.ReconstructOnNewPvec(pvec, i);
    pvecChangedFlag = false;
}

ConstraintType ConstraintArcLength::getTypeId()
{

Review comment (suggestion):
```suggestion
    *distance() = (h < *circle.rad) ? *circle.rad - h : h - *circle.rad;
```

Code after:
        if (param == distance()) {
            *grad = 1.0;
            if (length < *circle.rad) {
                *grad = -1.0;
            }
        }
        else if (param == circle.rad) {
            *grad = 1.0;
        }
        else {
            *grad = -dlength;
        }
    }
}
void ConstraintP2CDistance::evaluate()
{
    double h, dh;
    h = value(dh, nullptr);

    *distance() = (h < *circle.rad) ? *circle.rad - h : h - *circle.rad;
}

// --------------------------------------------------------
// ConstraintArcLength
ConstraintArcLength::ConstraintArcLength(Arc& a, double* d)
    : arc(a)
{
    pvec.push_back(d);
    this->arc.PushOwnParams(pvec);

    origpvec = pvec;
    pvecChangedFlag = true;
    rescale();
}

void ConstraintArcLength::ReconstructGeomPointers()
{
    int i = 0;
    i++;  // skip the first parameter as there is the inline function distance for it
    arc.ReconstructOnNewPvec(pvec, i);
    pvecChangedFlag = false;
}

ConstraintType ConstraintArcLength::getTypeId()
{
    return ArcLength;
}

void ConstraintArcLength::normalizedAngles(double& start, double& end) const
{
    end = *arc.endAngle;
Repository: FreeCAD/FreeCAD
File: src/Mod/Part/App/ExtrusionHelper.h (C/C++)

Code before:
#include <TopoDS_Shape.hxx>

#include <Mod/Part/PartGlobal.h>
#include "TopoShape.h"


namespace Part
{

class TopoShape;

/**
 * @brief The ExtrusionParameters struct is supposed to be filled with final
 * extrusion parameters, after resolving links, applying mode logic,
 * reversing, etc., and be passed to extrudeShape.
 */
struct ExtrusionParameters
{
    gp_Dir dir;
    double lengthFwd {0};
    double lengthRev {0};
    bool solid {false};
    double taperAngleFwd {0};  // in radians
    double taperAngleRev {0};
    std::string faceMakerClass;
    bool isPartDesign {false};  // affects inner wire taper handling for single-edge circles
};

class PartExport ExtrusionHelper
{
public:
    ExtrusionHelper();

    /**
     * @brief makeDraft: creates a drafted extrusion shape out of the input 2D shape
     */
    static void makeDraft(
        const TopoDS_Shape& shape,
        const gp_Dir& direction,
        const double LengthFwd,
        const double LengthRev,
        const double AngleFwd,
        const double AngleRev,
        bool isSolid,
        std::list<TopoDS_Shape>& drafts,
        bool isPartDesign
    );
    /**
     * @brief checkInnerWires: Checks what wires are inner ones by taking a set of prisms created
     * with every wire. The prisms are cut from each other. If the moment of inertia thereby
     * changes, the prism wire is an inner wire. Inner wires can have nested inner wires that are

Review comment (suggestion):
We should not make booleans like "isPartDesign" - "PartDesign" does not exists as a conecept in the domain of "Extrusion Paremeters". Instead we need to find the concept that part design does, name it and use here. 

Code after:
#include <gp_Dir.hxx>
#include <TopoDS_Shape.hxx>

#include <Mod/Part/PartGlobal.h>
#include "TopoShape.h"


namespace Part
{

class TopoShape;

/**
 * @brief Controls how taper is applied to inner wires (holes) during extrusion.
 */
enum class InnerWireTaper : std::uint8_t
{
    Inverted,
    SameAsOuter,
};

/**
 * @brief The ExtrusionParameters struct is supposed to be filled with final
 * extrusion parameters, after resolving links, applying mode logic,
 * reversing, etc., and be passed to extrudeShape.
 */
struct ExtrusionParameters
{
    gp_Dir dir;
    double lengthFwd {0};
    double lengthRev {0};
    bool solid {false};
    double taperAngleFwd {0};  // in radians
    double taperAngleRev {0};
    std::string faceMakerClass;
    InnerWireTaper innerWireTaper {InnerWireTaper::Inverted};
};

class PartExport ExtrusionHelper
{
public:
    ExtrusionHelper();

    /**
     * @brief makeDraft: creates a drafted extrusion shape out of the input 2D shape
     */
    static void makeDraft(
        const TopoDS_Shape& shape,
        const gp_Dir& direction,
        const double LengthFwd,
        const double LengthRev,
Repository: FreeCAD/FreeCAD
File: src/Mod/Sketcher/Gui/EditDatumDialog.cpp (C++)

Code before:

        return dlg.exec();
    }

    return QDialog::Rejected;
}

void EditDatumDialog::typeChanged(bool checked)
{
    if (!ui_ins_datum->rbRadius->isVisible()) {
        return;
    }

    // Updates UI labels based on selection, but does NOT change value yet
    QWidget* dlg = ui_ins_datum->labelEdit->parentWidget();
    while (dlg && !dlg->isWindow()) {
        dlg = dlg->parentWidget();
    }
    if (ui_ins_datum->rbRadius->isChecked()) {
        ui_ins_datum->label->setText(tr("Radius:"));
        if (dlg) {
            dlg->setWindowTitle(tr("Insert Radius"));
        }
    }
    else {
        ui_ins_datum->label->setText(tr("Diameter:"));
        if (dlg) {
            dlg->setWindowTitle(tr("Insert Diameter"));
        }
    }
}

void EditDatumDialog::accepted()
{
    // Check if we need to swap Radius <-> Diameter
    if (Constr->Type == Sketcher::Radius && ui_ins_datum->rbDiameter->isChecked()) {
        Constr->Type = Sketcher::Diameter;
    }
    else if (Constr->Type == Sketcher::Diameter && ui_ins_datum->rbRadius->isChecked()) {
        Constr->Type = Sketcher::Radius;
    }

    Base::Quantity newQuant = ui_ins_datum->labelEdit->value();
    if (Constr->Type == Sketcher::SnellsLaw || Constr->Type == Sketcher::Weight
        || !newQuant.isDimensionless()) {

        // save the value for the history
        ui_ins_datum->labelEdit->pushToHistory();

        double newDatum = newQuant.getValue();


Review comment (suggestion):
```suggestion
        ui_ins_datum->label->setText(tr("Diameter"));
```

Code after:

        return dlg.exec();
    }

    return QDialog::Rejected;
}

void EditDatumDialog::typeChanged(bool checked)
{
    if (!ui_ins_datum->rbRadius->isVisible()) {
        return;
    }

    // Updates UI labels based on selection, but does NOT change value yet
    QWidget* dlg = ui_ins_datum->labelEdit->parentWidget();
    while (dlg && !dlg->isWindow()) {
        dlg = dlg->parentWidget();
    }
    if (ui_ins_datum->rbRadius->isChecked()) {
        ui_ins_datum->label->setText(tr("Radius"));
        if (dlg) {
            dlg->setWindowTitle(tr("Insert Radius"));
        }
    }
    else {
        ui_ins_datum->label->setText(tr("Diameter"));
        if (dlg) {
            dlg->setWindowTitle(tr("Insert Diameter"));
        }
    }
}

void EditDatumDialog::accepted()
{
    // Check if we need to swap Radius <-> Diameter
    if (Constr->Type == Sketcher::Radius && ui_ins_datum->rbDiameter->isChecked()) {
        Constr->Type = Sketcher::Diameter;
    }
    else if (Constr->Type == Sketcher::Diameter && ui_ins_datum->rbRadius->isChecked()) {
        Constr->Type = Sketcher::Radius;
    }

    Base::Quantity newQuant = ui_ins_datum->labelEdit->value();
    if (Constr->Type == Sketcher::SnellsLaw || Constr->Type == Sketcher::Weight
        || !newQuant.isDimensionless()) {

        // save the value for the history
        ui_ins_datum->labelEdit->pushToHistory();

        double newDatum = newQuant.getValue();
Repository: FreeCAD/FreeCAD
File: src/Mod/Part/Gui/PreviewUpdateScheduler.h (C/C++)

Code before:
 ***************************************************************************/

#ifndef FREECAD_PREVIEWUPDATESCHEDULER_H
#define FREECAD_PREVIEWUPDATESCHEDULER_H

#include <QObject>
#include <QSet>

#include <Mod/Part/App/PreviewExtension.h>

namespace PartGui
{

class QtPreviewUpdateScheduler final: public QObject, public Part::PreviewUpdateScheduler
{
    Q_OBJECT

public:
    explicit QtPreviewUpdateScheduler(QObject* parent = nullptr);
    void schedulePreviewRecompute(Part::PreviewExtension* object) override;

private Q_SLOTS:
    void flush();

private:
    QSet<Part::PreviewExtension*> queue;
    bool scheduled = false;
};

}  // namespace PartGui

#endif  // FREECAD_PREVIEWUPDATESCHEDULER_H


Review comment (suggestion):
A `QSet` is not a queue. Is iteration order guaranteed to be insertion order? Otherwise you can use a `QQueue`.

Code after:
 ***************************************************************************/

#ifndef FREECAD_PREVIEWUPDATESCHEDULER_H
#define FREECAD_PREVIEWUPDATESCHEDULER_H

#include "App/DocumentObserver.h"


#include <QObject>
#include <QSet>

#include <Mod/Part/App/PreviewExtension.h>

namespace PartGui
{

/**
 * Qt-based implementation of the PreviewUpdateScheduler.
 *
 * This implementation uses the Qt Event Loop to debounce recompute requests.
 * Requests are queued and a flush is triggered via a queued connection,
 * ensuring that the actual update happens once the control returns to the
 * event loop after all pending property changes are processed.
 */
class QtPreviewUpdateScheduler final: public QObject, public Part::PreviewUpdateScheduler
{
    Q_OBJECT

public:
    explicit QtPreviewUpdateScheduler(QObject* parent = nullptr);

    /**
     * Schedules a preview recompute using the Qt Event Loop.
     *
     * Adds the object to a unique set to avoid duplicate work and schedules
     * a call to flush() using Qt::QueuedConnection if one isn't already pending.
     */
    void schedulePreviewRecompute(App::DocumentObject* object) override;

private Q_SLOTS:
    void flush();

private:
    std::unordered_set<App::DocumentObjectWeakPtrT> toBeUpdated;
    bool scheduled = false;
};

}  // namespace PartGui

#endif  // FREECAD_PREVIEWUPDATESCHEDULER_H
Repository: FreeCAD/FreeCAD
File: src/Mod/Part/App/GizmoHelper.h (C/C++)

Code before:
{
    Base::Vector3d position;
    Base::Vector3d dir;
    Base::Vector3d tangent;
};
DraggerPlacementProps PartExport
getDraggerPlacementFromEdgeAndFace(Part::TopoShape& edge, TopoDS_Face& face);

DraggerPlacementProps PartExport
getDraggerPlacementFromEdgeAndFace(Part::TopoShape& edge, Part::TopoShape& face);

std::vector<Part::TopoShape> PartExport getAdjacentEdgesFromFace(Part::TopoShape& face);

Base::Vector3d PartExport getMidPointFromFace(Part::TopoShape& face);

Base::Vector3d PartExport getMidPointFromProfile(Part::TopoShape& profile);

struct PartExport DraggerPlacementProps2
{
    Base::Vector3d position;
    Base::Vector3d dir;
    Base::Vector3d normal;
    Base::Vector3d faceNormal;
    // If this is false then normal and faceNormal are not defined
    bool normalDefined;
};
std::optional<DraggerPlacementProps2> PartExport
getDraggerPlacementFromPlaneAndFace(Part::TopoShape& face, gp_Pln& plane);

#endif /* GIZMO_HELPER_H */


Review comment (suggestion):
Such data structure really is

```
struct PositionAndDir {
    Base::Vector3d position;
    Base::Vector3d dir;
}

struct PositionWithNormals : PositionAndDir {
    Base::Vector3d normal;
    Base::Vector3d faceNormal;
}

using PositionWithOrWithoutNormals = std::variant<PositionAndDir, PositionWithNormals>;
```

Of course with better names. 

Not sure if this is worth here of course, but that would be a cleaner way in terms of architecture to solve it because it would force you to check and make it impossible to use `normal` if `normalDefined` is false. You can even wrap it additional std::optional (or add std::nullopt as possible variant) to have built-in optionality. 

Code after:
{
    Base::Vector3d position;
    Base::Vector3d dir;
};
DraggerPlacementProps PartExport
getDraggerPlacementFromEdgeAndFace(Part::TopoShape& edge, TopoDS_Face& face);

DraggerPlacementProps PartExport
getDraggerPlacementFromEdgeAndFace(Part::TopoShape& edge, Part::TopoShape& face);

std::vector<Part::TopoShape> PartExport getAdjacentEdgesFromFace(Part::TopoShape& face);

Base::Vector3d PartExport getMidPointFromFace(Part::TopoShape& face);

Base::Vector3d PartExport getMidPointFromProfile(Part::TopoShape& profile);

struct PartExport DraggerNormalProps
{
    Base::Vector3d normal;
    Base::Vector3d faceNormal;
};
struct PartExport DraggerPlacementPropsWithNormals
{
    DraggerPlacementProps placementProps;
    std::optional<DraggerNormalProps> normalProps;
};
std::optional<DraggerPlacementPropsWithNormals> PartExport
getDraggerPlacementFromPlaneAndFace(Part::TopoShape& face, gp_Pln& plane);

#endif /* GIZMO_HELPER_H */
Repository: FreeCAD/FreeCAD
File: src/Mod/CAM/libarea/Arc.cpp (C++)

Code before:
    double angs = atan2(m_s.y - m_c.y, m_s.x - m_c.x);
    double ange = atan2(m_e.y - m_c.y, m_e.x - m_c.x);
    if (m_dir) {
        // make sure ange > angs
        if (ange < angs) {
            ange += 6.2831853071795864;
        }
    }
    else {
        // make sure angs > ange
        if (angs < ange) {
            angs += 6.2831853071795864;
        }
    }

    return fabs(ange - angs);
}

bool CArc::AlmostALine() const
{
    Point mid_point = MidParam(0.5);
    if (Line(m_s, m_e - m_s).Dist(mid_point) <= CArea::get_accuracy()) {
        return true;
    }

    const double max_arc_radius = 1.0 / CArea::get_accuracy();
    double radius = m_c.dist(m_s);
    if (radius > max_arc_radius) {
        return true;  // We don't want to produce an arc whose radius is too large.
    }

    return false;
}

Point CArc::MidParam(double param) const
{
    /// returns a point which is 0-1 along arc
    if (fabs(param) < 0.00000000000001) {
        return m_s;
    }
    if (fabs(param - 1.0) < 0.00000000000001) {
        return m_e;
    }

    Point p;
    Point v = m_s - m_c;
    v.Rotate(param * IncludedAngle());
    p = v + m_c;

    return p;
}

Review comment (bug):
If set curve accuracy 0.01 mm, do not produce arcs with radius `1.0 / 0.01 = 100`

Increase value accuracy - decrease max allowed radius
accuracy 0.001 - allow radius 1000
accuracy 0.01 - allow radius 100
accuracy 0.1 - allow radius 10
accuracy 1 - allow radius 1

This looks wrong

Code after:
    double angs = atan2(m_s.y - m_c.y, m_s.x - m_c.x);
    double ange = atan2(m_e.y - m_c.y, m_e.x - m_c.x);
    if (m_dir) {
        // make sure ange > angs
        if (ange < angs) {
            ange += 6.2831853071795864;
        }
    }
    else {
        // make sure angs > ange
        if (angs < ange) {
            angs += 6.2831853071795864;
        }
    }

    return fabs(ange - angs);
}

bool CArc::AlmostALine() const
{
    Point mid_point = MidParam(0.5);
    if (Line(m_s, m_e - m_s).Dist(mid_point) <= CArea::get_accuracy()) {
        return true;
    }

    return false;
}

Point CArc::MidParam(double param) const
{
    /// returns a point which is 0-1 along arc
    if (fabs(param) < 0.00000000000001) {
        return m_s;
    }
    if (fabs(param - 1.0) < 0.00000000000001) {
        return m_e;
    }

    Point p;
    Point v = m_s - m_c;
    v.Rotate(param * IncludedAngle());
    p = v + m_c;

    return p;
}

// segments - number of segments per full revolution!
// d_angle - determines the direction and the amount of the arc to draw
void CArc::GetSegments(void (*callbackfunc)(const double* p), double pixels_per_mm) const
{
    if (m_s == m_e) {