From 1455321acef44e924f7c5587ae69e9d1ecc5b8a2 Mon Sep 17 00:00:00 2001 From: timpieces Date: Fri, 23 Jan 2026 11:58:30 +0800 Subject: [PATCH] ExpressionParser: Return unique_ptr from eval (#27098) - Most of the usages of 'release' are temporary here, but a few will likely need to stay. In these cases, the existing behaviour is retained. To fix all usages, smart pointers would need to be used all throughout the tree - The freecad_cast looks awkward now. I haven't added a new version that works with smart pointers because it's not clear what the right ownership semantics would be. Make the caller be explicit for now. --- src/App/Expression.cpp | 54 ++++++++++++++-------------- src/App/Expression.h | 2 +- src/App/ExpressionParser.h | 6 ++-- src/Mod/Part/App/AttachExtension.cpp | 4 +-- src/Mod/Spreadsheet/App/Sheet.cpp | 2 +- tests/src/App/Expression.cpp | 6 ++-- 6 files changed, 38 insertions(+), 36 deletions(-) diff --git a/src/App/Expression.cpp b/src/App/Expression.cpp index 6bbb237775..e13c7a2bd8 100644 --- a/src/App/Expression.cpp +++ b/src/App/Expression.cpp @@ -636,25 +636,31 @@ bool isAnyEqual(const App::any &v1, const App::any &v2) { return !!res; } -Expression* expressionFromPy(const DocumentObject *owner, const Py::Object &value) { +ExpressionPtr expressionFromPy(const DocumentObject* owner, const Py::Object& value) +{ if (value.isNone()) - return new PyObjectExpression(owner); + return std::make_unique(owner); if(value.isString()) { - return new StringExpression(owner,value.as_string()); + return std::make_unique(owner, value.as_string()); } else if (PyObject_TypeCheck(value.ptr(),&QuantityPy::Type)) { - return new NumberExpression(owner, - *static_cast(value.ptr())->getQuantityPtr()); + return std::make_unique( + owner, + *static_cast(value.ptr())->getQuantityPtr() + ); } else if (value.isBoolean()) { - if(value.isTrue()) - return new ConstantExpression(owner,"True",Quantity(1.0)); - else - return new ConstantExpression(owner,"False",Quantity(0.0)); + if (value.isTrue()) { + return std::make_unique(owner, "True", Quantity(1.0)); + } + else { + return std::make_unique(owner, "False", Quantity(0.0)); + } } else { Quantity q; - if(pyToQuantity(q,value)) - return new NumberExpression(owner,q); + if (pyToQuantity(q, value)) { + return std::make_unique(owner, q); + } } - return new PyObjectExpression(owner,value.ptr()); + return std::make_unique(owner, value.ptr()); } } // namespace App @@ -686,12 +692,7 @@ Expression::Component::Component(const Component &other) ,e3(other.e3?other.e3->copy():nullptr) {} -Expression::Component::~Component() -{ - delete e1; - delete e2; - delete e3; -} +Expression::Component::~Component() = default; Expression::Component* Expression::Component::copy() const { return new Component(*this); @@ -1135,9 +1136,10 @@ void Expression::visit(ExpressionVisitor &v) { v.visit(*this); } -Expression* Expression::eval() const { +ExpressionPtr Expression::eval() const +{ Base::PyGILStateLocker lock; - return expressionFromPy(owner,getPyValue()); + return expressionFromPy(owner, getPyValue()); } bool Expression::isSame(const Expression &other, bool checkComment) const { @@ -1445,7 +1447,7 @@ Expression *OperatorExpression::simplify() const if (freecad_cast(v1) && freecad_cast(v2)) { delete v1; delete v2; - return eval(); + return eval().release(); } else return new OperatorExpression(owner, v1, op, v2); @@ -2605,7 +2607,7 @@ Expression *FunctionExpression::simplify() const for (auto it : simplifiedArgs) delete it; - return eval(); + return eval().release(); } else return new FunctionExpression(owner, f, std::string(fname), @@ -2824,16 +2826,16 @@ void VariableExpression::addComponent(Component *c) { } long l1=0,l2=0,l3=1; if(c->e3) { - auto n3 = freecad_cast(c->e3); + auto n3 = freecad_cast(c->e3.get()); if(!n3 || !essentiallyEqual(n3->getValue(),(double)l3)) break; } if(c->e1) { - auto n1 = freecad_cast(c->e1); + auto n1 = freecad_cast(c->e1.get()); if(!n1) { if(c->e2 || c->e3) break; - auto s = freecad_cast(c->e1); + auto s = freecad_cast(c->e1.get()); if(!s) break; var << ObjectIdentifier::MapComponent( @@ -2850,7 +2852,7 @@ void VariableExpression::addComponent(Component *c) { return; } } - auto n2 = freecad_cast(c->e2); + auto n2 = freecad_cast(c->e2.get()); if(n2 && essentiallyInteger(n2->getValue(),l2)) { var << ObjectIdentifier::RangeComponent(l1,l2,l3); return; diff --git a/src/App/Expression.h b/src/App/Expression.h index bdc4512b7c..d8e0a0902f 100644 --- a/src/App/Expression.h +++ b/src/App/Expression.h @@ -332,7 +332,7 @@ public: * * @return The evaluated expression. */ - Expression* eval() const; + ExpressionPtr eval() const; /** * @brief Convert the expression to a string. diff --git a/src/App/ExpressionParser.h b/src/App/ExpressionParser.h index eef993eb05..4579fa011a 100644 --- a/src/App/ExpressionParser.h +++ b/src/App/ExpressionParser.h @@ -48,9 +48,9 @@ namespace App struct AppExport Expression::Component { ObjectIdentifier::Component comp; - Expression* e1; - Expression* e2; - Expression* e3; + ExpressionPtr e1; + ExpressionPtr e2; + ExpressionPtr e3; explicit Component(const std::string& n); Component(Expression* e1, Expression* e2, Expression* e3, bool isRange = false); diff --git a/src/Mod/Part/App/AttachExtension.cpp b/src/Mod/Part/App/AttachExtension.cpp index 248bb7811f..5994b5bdb5 100644 --- a/src/Mod/Part/App/AttachExtension.cpp +++ b/src/Mod/Part/App/AttachExtension.cpp @@ -621,8 +621,8 @@ void AttachExtension::handleLegacyTangentPlaneOrientation() unitSafeExprStr += " - " + std::to_string(-angle); } - if (const App::Expression* simple = expr->eval()) { - if (auto ue = dynamic_cast(simple)) { + if (App::ExpressionPtr simple = expr->eval(); simple) { + if (auto ue = dynamic_cast(simple.get())) { const auto& q = ue->getQuantity(); if (q.getUnit() == Base::Unit::Angle) { unitSafeExprStr += " deg"; diff --git a/src/Mod/Spreadsheet/App/Sheet.cpp b/src/Mod/Spreadsheet/App/Sheet.cpp index df27045ea7..cd4adc49b7 100644 --- a/src/Mod/Spreadsheet/App/Sheet.cpp +++ b/src/Mod/Spreadsheet/App/Sheet.cpp @@ -788,7 +788,7 @@ void Sheet::updateProperty(CellAddress key) if (input) { CurrentAddressLock lock(currentRow, currentCol, key); - output.reset(input->eval()); + output = input->eval(); } else { std::string s; diff --git a/tests/src/App/Expression.cpp b/tests/src/App/Expression.cpp index 59994d4e61..56db683e03 100644 --- a/tests/src/App/Expression.cpp +++ b/tests/src/App/Expression.cpp @@ -364,7 +364,7 @@ private: TEST_F(Evaluate, test_evaluate_simple) { App::ExpressionPtr e = App::ExpressionParser::parse(this_obj(), "1 + 2"); - App::Expression* evaluated = e->eval(); + App::ExpressionPtr evaluated = e->eval(); EXPECT_EQ(e->toString(), "1 + 2"); EXPECT_EQ(evaluated->toString(), "3"); } @@ -380,7 +380,7 @@ TEST_F(Evaluate, test_simplify_simple) TEST_F(Evaluate, test_evaluate_function) { App::ExpressionPtr e = App::ExpressionParser::parse(this_obj(), "sqrt(4)"); - App::Expression* evaluated = e->eval(); + App::ExpressionPtr evaluated = e->eval(); EXPECT_EQ(e->toString(), "sqrt(4)"); EXPECT_EQ(evaluated->toString(), "2"); } @@ -398,7 +398,7 @@ TEST_F(Evaluate, test_evaluate_refer_to_var) auto* prop = freecad_cast(this_obj()->addDynamicProperty("App::PropertyFloat", "Var")); prop->setValue(2.0); App::ExpressionPtr e = App::ExpressionParser::parse(this_obj(), "sqrt(2 + Var)"); - App::Expression* evaluated = e->eval(); + App::ExpressionPtr evaluated = e->eval(); EXPECT_EQ(e->toString(), "sqrt(2 + Var)"); EXPECT_EQ(evaluated->toString(), "2"); }