From 192bd95e66a772fd659666759695af0b77e1d9e5 Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Wed, 8 Jul 2026 14:20:29 -0700 Subject: [PATCH] Simplify program layout for variadic and/or. PiperOrigin-RevId: 944697041 --- eval/compiler/flat_expr_builder.cc | 20 +++++++++++++------- eval/eval/BUILD | 1 - eval/eval/jump_step.cc | 7 ++++--- eval/eval/jump_step.h | 3 ++- eval/eval/logic_step.cc | 10 ++++++---- eval/eval/logic_step.h | 6 ++++-- eval/eval/logic_step_test.cc | 4 +++- 7 files changed, 32 insertions(+), 19 deletions(-) diff --git a/eval/compiler/flat_expr_builder.cc b/eval/compiler/flat_expr_builder.cc index fda57ca73..fc7289bd2 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -2172,21 +2172,27 @@ void LogicalCondVisitor::PostVisitArg(int arg_num, const cel::Expr* expr) { return; } const int last_arg_index = expr->call_expr().args().size() - 1; - if (arg_num > 0) { + const size_t num_args = expr->call_expr().args().size(); + if (arg_num == last_arg_index) { if (is_or_) { - visitor_->AddStep(CreateOrStep(expr->id())); + visitor_->AddStep(CreateOrStep(num_args, expr->id())); } else { - visitor_->AddStep(CreateAndStep(expr->id())); + visitor_->AddStep(CreateAndStep(num_args, expr->id())); } if (short_circuiting_ && !jump_steps_.empty()) { - visitor_->SetProgressStatusIfError( - jump_steps_.back().set_target(visitor_->GetCurrentIndex())); + for (auto& jump : jump_steps_) { + visitor_->SetProgressStatusIfError( + jump.set_target(visitor_->GetCurrentIndex())); + } } } if (short_circuiting_ && arg_num < last_arg_index) { std::unique_ptr jump_step = - is_or_ ? CreateCondJumpStep(true, {}, expr->id()) - : CreateCondJumpStep(false, {}, expr->id()); + is_or_ + ? CreateCondJumpStep(true, {}, /*expected_stack_size=*/arg_num + 1, + expr->id()) + : CreateCondJumpStep(false, {}, /*expected_stack_size=*/arg_num + 1, + expr->id()); ProgramStepIndex index = visitor_->GetCurrentIndex(); if (JumpStepBase* jump_step_ptr = visitor_->AddStep(std::move(jump_step)); jump_step_ptr) { diff --git a/eval/eval/BUILD b/eval/eval/BUILD index 44c7ded79..c0f544405 100644 --- a/eval/eval/BUILD +++ b/eval/eval/BUILD @@ -418,7 +418,6 @@ cc_library( "//common:value", "//eval/internal:errors", "@com_google_absl//absl/status", - "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/types:optional", "@com_google_cel_spec//proto/cel/expr:syntax_cc_proto", ], diff --git a/eval/eval/jump_step.cc b/eval/eval/jump_step.cc index 60c8c039e..243a02e8a 100644 --- a/eval/eval/jump_step.cc +++ b/eval/eval/jump_step.cc @@ -143,9 +143,10 @@ class BoolCheckJumpStep : public JumpStepBase { // Factory method for Conditional Jump step. std::unique_ptr CreateCondJumpStep( - bool jump_condition, absl::optional jump_offset, int64_t expr_id) { - return std::make_unique(jump_condition, jump_offset, 1, - expr_id); + bool jump_condition, absl::optional jump_offset, + size_t expected_stack_size, int64_t expr_id) { + return std::make_unique(jump_condition, jump_offset, + expected_stack_size, expr_id); } // Factory method for Ternary Conditional Jump step. diff --git a/eval/eval/jump_step.h b/eval/eval/jump_step.h index 27deba352..d8555ae10 100644 --- a/eval/eval/jump_step.h +++ b/eval/eval/jump_step.h @@ -53,7 +53,8 @@ std::unique_ptr CreateJumpStep(absl::optional jump_offset, // It is compared to jump_condition, and if matched, jump is performed. // The boolean value is left on top of the stack. std::unique_ptr CreateCondJumpStep( - bool jump_condition, absl::optional jump_offset, int64_t expr_id); + bool jump_condition, absl::optional jump_offset, + size_t expected_stack_size, int64_t expr_id); // Factory method for Ternary Conditional Jump step. // Requires a boolean condition value on top of the stack. diff --git a/eval/eval/logic_step.cc b/eval/eval/logic_step.cc index 6d5c8bbb3..9e65db8f0 100644 --- a/eval/eval/logic_step.cc +++ b/eval/eval/logic_step.cc @@ -450,13 +450,15 @@ std::unique_ptr CreateDirectOrStep( } // Factory method for "And" Execution step -absl::StatusOr> CreateAndStep(int64_t expr_id) { - return std::make_unique(OpType::kAnd, 2, expr_id); +absl::StatusOr> CreateAndStep(size_t num_args, + int64_t expr_id) { + return std::make_unique(OpType::kAnd, num_args, expr_id); } // Factory method for "Or" Execution step -absl::StatusOr> CreateOrStep(int64_t expr_id) { - return std::make_unique(OpType::kOr, 2, expr_id); +absl::StatusOr> CreateOrStep(size_t num_args, + int64_t expr_id) { + return std::make_unique(OpType::kOr, num_args, expr_id); } // Factory method for recursive logical not "!" Execution step diff --git a/eval/eval/logic_step.h b/eval/eval/logic_step.h index d75ed3715..4f5be2615 100644 --- a/eval/eval/logic_step.h +++ b/eval/eval/logic_step.h @@ -23,10 +23,12 @@ std::unique_ptr CreateDirectOrStep( bool shortcircuiting); // Factory method for "And" Execution step -absl::StatusOr> CreateAndStep(int64_t expr_id); +absl::StatusOr> CreateAndStep(size_t num_args, + int64_t expr_id); // Factory method for "Or" Execution step -absl::StatusOr> CreateOrStep(int64_t expr_id); +absl::StatusOr> CreateOrStep(size_t num_args, + int64_t expr_id); // Factory method for recursive logical not "!" Execution step std::unique_ptr CreateDirectNotStep( diff --git a/eval/eval/logic_step_test.cc b/eval/eval/logic_step_test.cc index 17ca8ba0d..2bdcbc8ea 100644 --- a/eval/eval/logic_step_test.cc +++ b/eval/eval/logic_step_test.cc @@ -73,7 +73,9 @@ class LogicStepTest : public testing::TestWithParam { CEL_ASSIGN_OR_RETURN(step, CreateIdentStep("name1", /*expr_id=*/-1)); path.push_back(std::move(step)); - CEL_ASSIGN_OR_RETURN(step, (is_or) ? CreateOrStep(2) : CreateAndStep(2)); + CEL_ASSIGN_OR_RETURN( + step, (is_or) ? CreateOrStep(/*num_args=*/2, /*expr_id=*/2) + : CreateAndStep(/*num_args=*/2, /*expr_id=*/2)); path.push_back(std::move(step)); auto dummy_expr = std::make_unique();