Use simplify_exprtt::resultt in pre-order simplification steps

The use of resultt increases type safety as the expression to be
simplified is no longer modified in place. All post-order simplification
steps already use resultt, but pre-order steps had been left to be
done.

Author: tautschnig

src/util/simplify_expr.cpp

@@ -816,9 +816,8 @@ simplify_exprt::simplify_typecast(const typecast_exprt &expr)
     // rewrite (T)(bool) to bool?1:0
     auto one = from_integer(1, expr_type);
     auto zero = from_integer(0, expr_type);
-    exprt new_expr = if_exprt(expr.op(), std::move(one), std::move(zero));
-    simplify_if_preorder(to_if_expr(new_expr));
-    return new_expr;
+    return changed(simplify_if_preorder(
+      if_exprt{expr.op(), std::move(one), std::move(zero)}));
   }
 
   // circular casts through types shorter than `int`
@@ -1336,33 +1335,33 @@ simplify_exprt::simplify_typecast(const typecast_exprt &expr)
   return unchanged(expr);
 }
 
-bool simplify_exprt::simplify_typecast_preorder(typecast_exprt &expr)
+simplify_exprt::resultt<>
+simplify_exprt::simplify_typecast_preorder(const typecast_exprt &expr)
 {
-  const typet &expr_type = as_const(expr).type();
-  const typet &op_type = as_const(expr).op().type();
+  const typet &expr_type = expr.type();
+  const typet &op_type = expr.op().type();
 
   // (T)(a?b:c) --> a?(T)b:(T)c; don't do this for floating-point type casts as
   // the type cast itself may be costly
   if(
-    as_const(expr).op().id() == ID_if && expr_type.id() != ID_floatbv &&
+    expr.op().id() == ID_if && expr_type.id() != ID_floatbv &&
     op_type.id() != ID_floatbv)
   {
     if_exprt if_expr = lift_if(expr, 0);
-    simplify_if_preorder(if_expr);
-    expr.swap(if_expr);
-    return false;
+    return changed(simplify_if_preorder(if_expr));
   }
   else
   {
     auto r_it = simplify_rec(expr.op()); // recursive call
     if(r_it.has_changed())
     {
-      expr.op() = r_it.expr;
-      return false;
+      auto tmp = expr;
+      tmp.op() = r_it.expr;
+      return tmp;
     }
-    else
-      return true;
   }
+
+  return unchanged(expr);
 }
 
 simplify_exprt::resultt<>
@@ -2629,40 +2628,50 @@ simplify_exprt::simplify_overflow_result(const overflow_result_exprt &expr)
   }
 }
 
-bool simplify_exprt::simplify_node_preorder(exprt &expr)
+simplify_exprt::resultt<>
+simplify_exprt::simplify_node_preorder(const exprt &expr)
 {
-  bool result=true;
-
   // The ifs below could one day be replaced by a switch()
 
   if(expr.id()==ID_address_of)
   {
     // the argument of this expression needs special treatment
   }
   else if(expr.id()==ID_if)
-    result=simplify_if_preorder(to_if_expr(expr));
+  {
+    return simplify_if_preorder(to_if_expr(expr));
+  }
   else if(expr.id() == ID_typecast)
-    result = simplify_typecast_preorder(to_typecast_expr(expr));
-  else
   {
-    if(expr.has_operands())
+    return simplify_typecast_preorder(to_typecast_expr(expr));
+  }
+  else if(expr.has_operands())
+  {
+    optionalt<exprt::operandst> new_operands;
+
+    for(std::size_t i = 0; i < expr.operands().size(); ++i)
     {
-      Forall_operands(it, expr)
+      auto r_it = simplify_rec(expr.operands()[i]); // recursive call
+      if(r_it.has_changed())
       {
-        auto r_it = simplify_rec(*it); // recursive call
-        if(r_it.has_changed())
-        {
-          *it = r_it.expr;
-          result=false;
-        }
+        if(!new_operands.has_value())
+          new_operands = expr.operands();
+        (*new_operands)[i] = std::move(r_it.expr);
       }
     }
+
+    if(new_operands.has_value())
+    {
+      exprt result = expr;
+      std::swap(result.operands(), *new_operands);
+      return result;
+    }
   }
 
-  return result;
+  return unchanged(expr);
 }
 
-simplify_exprt::resultt<> simplify_exprt::simplify_node(exprt node)
+simplify_exprt::resultt<> simplify_exprt::simplify_node(const exprt &node)
 {
   if(!node.has_operands())
     return unchanged(node); // no change
@@ -2947,50 +2956,52 @@ simplify_exprt::resultt<> simplify_exprt::simplify_rec(const exprt &expr)
   #endif
 
   // We work on a copy to prevent unnecessary destruction of sharing.
-  exprt tmp=expr;
-  bool no_change = simplify_node_preorder(tmp);
+  auto simplify_node_preorder_result = simplify_node_preorder(expr);
 
-  auto simplify_node_result = simplify_node(tmp);
+  auto simplify_node_result = simplify_node(simplify_node_preorder_result.expr);
 
-  if(simplify_node_result.has_changed())
+  if(
+    !simplify_node_result.has_changed() &&
+    simplify_node_preorder_result.has_changed())
   {
-    no_change = false;
-    tmp = simplify_node_result.expr;
+    simplify_node_result.expr_changed =
+      simplify_node_preorder_result.expr_changed;
   }
 
 #ifdef USE_LOCAL_REPLACE_MAP
-  #if 1
-  replace_mapt::const_iterator it=local_replace_map.find(tmp);
+  exprt tmp = simplify_node_result.expr;
+#  if 1
+  replace_mapt::const_iterator it =
+    local_replace_map.find(simplify_node_result.expr);
   if(it!=local_replace_map.end())
+    simplify_node_result = changed(it->second);
+#  else
+  if(
+    !local_replace_map.empty() &&
+    !replace_expr(local_replace_map, simplify_node_result.expr))
   {
-    tmp=it->second;
-    no_change = false;
-  }
-  #else
-  if(!local_replace_map.empty() &&
-     !replace_expr(local_replace_map, tmp))
-  {
-    simplify_rec(tmp);
-    no_change = false;
+    simplify_node_result = changed(simplify_rec(simplify_node_result.expr));
   }
-  #endif
+#  endif
 #endif
 
-  if(no_change) // no change
+  if(!simplify_node_result.has_changed())
   {
     return unchanged(expr);
   }
-  else // change, new expression is 'tmp'
+  else
   {
     POSTCONDITION_WITH_DIAGNOSTICS(
-      as_const(tmp).type() == expr.type(), tmp.pretty(), expr.pretty());
+      as_const(simplify_node_result.expr).type() == expr.type(),
+      simplify_node_result.expr.pretty(),
+      expr.pretty());
 
 #ifdef USE_CACHE
     // save in cache
-    cache_result.first->second = tmp;
+    cache_result.first->second = simplify_node_result.expr;
 #endif
 
-    return std::move(tmp);
+    return simplify_node_result;
   }
 }
 

src/util/simplify_expr_class.h

@@ -147,7 +147,7 @@ class simplify_exprt
   // These below all return 'true' if the simplification wasn't applicable.
   // If false is returned, the expression has changed.
   NODISCARD resultt<> simplify_typecast(const typecast_exprt &);
-  bool simplify_typecast_preorder(typecast_exprt &);
+  NODISCARD resultt<> simplify_typecast_preorder(const typecast_exprt &);
   NODISCARD resultt<> simplify_extractbit(const extractbit_exprt &);
   NODISCARD resultt<> simplify_extractbits(const extractbits_exprt &);
   NODISCARD resultt<> simplify_concatenation(const concatenation_exprt &);
@@ -161,7 +161,7 @@ class simplify_exprt
   NODISCARD resultt<> simplify_shifts(const shift_exprt &);
   NODISCARD resultt<> simplify_power(const binary_exprt &);
   NODISCARD resultt<> simplify_bitwise(const multi_ary_exprt &);
-  bool simplify_if_preorder(if_exprt &expr);
+  NODISCARD resultt<> simplify_if_preorder(const if_exprt &expr);
   NODISCARD resultt<> simplify_if(const if_exprt &);
   NODISCARD resultt<> simplify_bitnot(const bitnot_exprt &);
   NODISCARD resultt<> simplify_not(const not_exprt &);
@@ -249,8 +249,8 @@ class simplify_exprt
   simplify_inequality_pointer_object(const binary_relation_exprt &);
 
   // main recursion
-  NODISCARD resultt<> simplify_node(exprt);
-  bool simplify_node_preorder(exprt &expr);
+  NODISCARD resultt<> simplify_node(const exprt &);
+  NODISCARD resultt<> simplify_node_preorder(const exprt &);
   NODISCARD resultt<> simplify_rec(const exprt &);
 
   virtual bool simplify(exprt &expr);