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

@@ -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<PyObjectExpression>(owner);
     if(value.isString()) {
-        return new StringExpression(owner,value.as_string());
+        return std::make_unique<StringExpression>(owner, value.as_string());
     } else if (PyObject_TypeCheck(value.ptr(),&QuantityPy::Type)) {
-        return new NumberExpression(owner,
-                *static_cast<QuantityPy*>(value.ptr())->getQuantityPtr());
+        return std::make_unique<NumberExpression>(
+            owner,
+            *static_cast<QuantityPy*>(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<ConstantExpression>(owner, "True", Quantity(1.0));
+        }
+        else {
+            return std::make_unique<ConstantExpression>(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<NumberExpression>(owner, q);
+        }
     }
-    return new PyObjectExpression(owner,value.ptr());
+    return std::make_unique<PyObjectExpression>(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<NumberExpression*>(v1) && freecad_cast<NumberExpression*>(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<NumberExpression*>(c->e3);
+            auto n3 = freecad_cast<NumberExpression*>(c->e3.get());
             if(!n3 || !essentiallyEqual(n3->getValue(),(double)l3))
                 break;
         }
         if(c->e1) {
-            auto n1 = freecad_cast<NumberExpression*>(c->e1);
+            auto n1 = freecad_cast<NumberExpression*>(c->e1.get());
             if(!n1) {
                 if(c->e2 || c->e3)
                     break;
-                auto s = freecad_cast<StringExpression*>(c->e1);
+                auto s = freecad_cast<StringExpression*>(c->e1.get());
                 if(!s)
                     break;
                 var << ObjectIdentifier::MapComponent(
@@ -2850,7 +2852,7 @@ void VariableExpression::addComponent(Component *c) {
                 return;
             }
         }
-        auto n2 = freecad_cast<NumberExpression*>(c->e2);
+        auto n2 = freecad_cast<NumberExpression*>(c->e2.get());
         if(n2 && essentiallyInteger(n2->getValue(),l2)) {
             var << ObjectIdentifier::RangeComponent(l1,l2,l3);
             return;

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.

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);

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<const App::UnitExpression*>(simple)) {
+            if (App::ExpressionPtr simple = expr->eval(); simple) {
+                if (auto ue = dynamic_cast<const App::UnitExpression*>(simple.get())) {
                     const auto& q = ue->getQuantity();
                     if (q.getUnit() == Base::Unit::Angle) {
                         unitSafeExprStr += " deg";

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;

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<App::PropertyFloat*>(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");
 }