SERVER-39397 Simplify Walker to remove reference parameters

2 files changed, 20 insertions, 53 deletions

diff --git a/src/mongo/db/pipeline/expression_walker.h b/src/mongo/db/pipeline/expression_walker.h
index 7c6097dc562..3222d02ceb9 100644
--- a/src/mongo/db/pipeline/expression_walker.h
+++ b/src/mongo/db/pipeline/expression_walker.h
@@ -42,28 +42,24 @@ namespace mongo::expression_walker {
  *   pair of children. walker.inVisit() is skipped if the Expression has fewer than two children.
  * * walker.postVisit() once after walking to each child.
  * Each of the Expression's child Expressions is recursively walked and the same three methods are
- * called for it. preVisit() and postVisit() must return a pointer to an Expression. If either does,
- * walk() will replace the current Expression with the return value. If no change is needed during a
- * particular call, preVisit() and postVisit() may return their argument.
+ * called for it.
  */
 template <typename Walker>
-void walk(Walker& walker, boost::intrusive_ptr<Expression>& expression) {
+void walk(Walker* walker, Expression* expression) {
     if (expression) {
-        expression = walker.preVisit(expression);
+        walker->preVisit(expression);
 
         // InVisit needs to be called between every two nodes which requires more complicated
-        // branching logic. InVisit is forbidden from replacing its Expression through the return
-        // value and must return void since it would break our iteration and be confusing to
-        // replace a node while only a portion of its children have been walked.
+        // branching logic.
         auto count = 0ull;
         for (auto&& child : expression->getChildren()) {
             if (count != 0ull)
-                walker.inVisit(count, expression);
+                walker->inVisit(count, expression);
             ++count;
-            walk(walker, child);
+            walk(walker, child.get());
         }
 
-        expression = walker.postVisit(expression);
+        walker->postVisit(expression);
     }
 }
 
diff --git a/src/mongo/db/pipeline/expression_walker_test.cpp b/src/mongo/db/pipeline/expression_walker_test.cpp
index 05360a46160..4cc5a1c4691 100644
--- a/src/mongo/db/pipeline/expression_walker_test.cpp
+++ b/src/mongo/db/pipeline/expression_walker_test.cpp
@@ -67,34 +67,28 @@ using namespace expression_walker;
 
 TEST_F(ExpressionWalkerTest, NullTreeWalkSucceeds) {
     struct {
-        boost::intrusive_ptr<Expression> preVisit(boost::intrusive_ptr<Expression>& expression) {
-            return expression;
-        }
-        void inVisit(unsigned long long, boost::intrusive_ptr<Expression>&) {}
-        boost::intrusive_ptr<Expression> postVisit(boost::intrusive_ptr<Expression>& expression) {
-            return expression;
-        }
+        void preVisit(Expression*) {}
+        void inVisit(unsigned long long, Expression*) {}
+        void postVisit(Expression*) {}
     } nothingWalker;
     auto expression = boost::intrusive_ptr<Expression>();
-    walk(nothingWalker, expression);
+    walk(&nothingWalker, expression.get());
 }
 
 TEST_F(ExpressionWalkerTest, PrintWalkReflectsMutation) {
     struct {
-        boost::intrusive_ptr<Expression> preVisit(boost::intrusive_ptr<Expression>& expression) {
+        void preVisit(Expression* expression) {
             if (typeid(*expression) == typeid(ExpressionConcat))
                 string += "{$concat: [";
-            if (auto constant = dynamic_cast<ExpressionConstant*>(expression.get()))
+            if (auto constant = dynamic_cast<ExpressionConstant*>(expression))
                 string += "\""s + constant->getValue().getString() + "\"";
-            return expression;
         }
-        void inVisit(unsigned long long, boost::intrusive_ptr<Expression>& expression) {
+        void inVisit(unsigned long long, Expression* expression) {
             string += ", ";
         }
-        boost::intrusive_ptr<Expression> postVisit(boost::intrusive_ptr<Expression>& expression) {
+        void postVisit(Expression* expression) {
             if (typeid(*expression) == typeid(ExpressionConcat))
                 string += "]}";
-            return expression;
         }
 
         std::string string;
@@ -102,46 +96,23 @@ TEST_F(ExpressionWalkerTest, PrintWalkReflectsMutation) {
 
     auto expressionString = "{$concat: [\"black\", \"green\", \"yellow\"]}"s;
     auto expression = parseExpression(expressionString);
-    walk(stringWalker, expression);
+    walk(&stringWalker, expression.get());
     ASSERT_EQ(stringWalker.string, expressionString);
-
-    struct {
-        boost::intrusive_ptr<Expression> preVisit(boost::intrusive_ptr<Expression>& expression) {
-            if (auto constant = dynamic_cast<ExpressionConstant*>(expression.get()))
-                if (constant->getValue().getString() == "black")
-                    return ExpressionConstant::create(expCtx, Value("white"s));
-            return expression;
-        }
-        void inVisit(unsigned long long, boost::intrusive_ptr<Expression>& expression) {}
-        boost::intrusive_ptr<Expression> postVisit(boost::intrusive_ptr<Expression>& expression) {
-            return expression;
-        }
-        const boost::intrusive_ptr<ExpressionContext>& expCtx;
-    } whiteWalker{getExpCtx()};
-
-    walk(whiteWalker, expression);
-    stringWalker.string.clear();
-    walk(stringWalker, expression);
-    ASSERT_EQ(stringWalker.string, "{$concat: [\"white\", \"green\", \"yellow\"]}"s);
 }
 
 TEST_F(ExpressionWalkerTest, InVisitCanCount) {
     struct {
-        boost::intrusive_ptr<Expression> preVisit(boost::intrusive_ptr<Expression>& expression) {
-            return expression;
-        }
-        void inVisit(unsigned long long count, boost::intrusive_ptr<Expression>&) {
+        void preVisit(Expression*) {}
+        void inVisit(unsigned long long count, Expression*) {
             counter.push_back(count);
         }
-        boost::intrusive_ptr<Expression> postVisit(boost::intrusive_ptr<Expression>& expression) {
-            return expression;
-        }
+        void postVisit(Expression*) {}
         std::vector<unsigned long long> counter;
     } countWalker;
 
     auto expressionString = "{$and: [true, false, true, true, false, true]}"s;
     auto expression = parseExpression(expressionString);
-    walk(countWalker, expression);
+    walk(&countWalker, expression.get());
     ASSERT(countWalker.counter == std::vector({1ull, 2ull, 3ull, 4ull, 5ull}));
 }