Skip to content

Commit bc6a2e2

Browse files
jnthntatumcopybara-github
jnthntatum
authored and
copybara-github
committed
Simplify program layout for variadic and/or.
PiperOrigin-RevId: 945926874
1 parent 05556a0 commit bc6a2e2

7 files changed

Lines changed: 32 additions & 19 deletions

File tree

‎eval/compiler/flat_expr_builder.cc‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2172,21 +2172,27 @@ void LogicalCondVisitor::PostVisitArg(int arg_num, const cel::Expr* expr) {
21722172
return;
21732173
}
21742174
const int last_arg_index = expr->call_expr().args().size() - 1;
2175-
if (arg_num > 0) {
2175+
const size_t num_args = expr->call_expr().args().size();
2176+
if (arg_num == last_arg_index) {
21762177
if (is_or_) {
2177-
visitor_->AddStep(CreateOrStep(expr->id()));
2178+
visitor_->AddStep(CreateOrStep(num_args, expr->id()));
21782179
} else {
2179-
visitor_->AddStep(CreateAndStep(expr->id()));
2180+
visitor_->AddStep(CreateAndStep(num_args, expr->id()));
21802181
}
21812182
if (short_circuiting_ && !jump_steps_.empty()) {
2182-
visitor_->SetProgressStatusIfError(
2183-
jump_steps_.back().set_target(visitor_->GetCurrentIndex()));
2183+
for (auto& jump : jump_steps_) {
2184+
visitor_->SetProgressStatusIfError(
2185+
jump.set_target(visitor_->GetCurrentIndex()));
2186+
}
21842187
}
21852188
}
21862189
if (short_circuiting_ && arg_num < last_arg_index) {
21872190
std::unique_ptr<JumpStepBase> jump_step =
2188-
is_or_ ? CreateCondJumpStep(true, {}, expr->id())
2189-
: CreateCondJumpStep(false, {}, expr->id());
2191+
is_or_
2192+
? CreateCondJumpStep(true, {}, /*expected_stack_size=*/arg_num + 1,
2193+
expr->id())
2194+
: CreateCondJumpStep(false, {}, /*expected_stack_size=*/arg_num + 1,
2195+
expr->id());
21902196
ProgramStepIndex index = visitor_->GetCurrentIndex();
21912197
if (JumpStepBase* jump_step_ptr = visitor_->AddStep(std::move(jump_step));
21922198
jump_step_ptr) {

‎eval/eval/BUILD‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -418,7 +418,6 @@ cc_library(
418418
"//common:value",
419419
"//eval/internal:errors",
420420
"@com_google_absl//absl/status",
421-
"@com_google_absl//absl/status:statusor",
422421
"@com_google_absl//absl/types:optional",
423422
"@com_google_cel_spec//proto/cel/expr:syntax_cc_proto",
424423
],

‎eval/eval/jump_step.cc‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -143,9 +143,10 @@ class BoolCheckJumpStep : public JumpStepBase {
143143

144144
// Factory method for Conditional Jump step.
145145
std::unique_ptr<JumpStepBase> CreateCondJumpStep(
146-
bool jump_condition, absl::optional<int> jump_offset, int64_t expr_id) {
147-
return std::make_unique<CondJumpStep>(jump_condition, jump_offset, 1,
148-
expr_id);
146+
bool jump_condition, absl::optional<int> jump_offset,
147+
size_t expected_stack_size, int64_t expr_id) {
148+
return std::make_unique<CondJumpStep>(jump_condition, jump_offset,
149+
expected_stack_size, expr_id);
149150
}
150151

151152
// Factory method for Ternary Conditional Jump step.

‎eval/eval/jump_step.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,8 @@ std::unique_ptr<JumpStepBase> CreateJumpStep(absl::optional<int> jump_offset,
5353
// It is compared to jump_condition, and if matched, jump is performed.
5454
// The boolean value is left on top of the stack.
5555
std::unique_ptr<JumpStepBase> CreateCondJumpStep(
56-
bool jump_condition, absl::optional<int> jump_offset, int64_t expr_id);
56+
bool jump_condition, absl::optional<int> jump_offset,
57+
size_t expected_stack_size, int64_t expr_id);
5758

5859
// Factory method for Ternary Conditional Jump step.
5960
// Requires a boolean condition value on top of the stack.

‎eval/eval/logic_step.cc‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -450,13 +450,15 @@ std::unique_ptr<DirectExpressionStep> CreateDirectOrStep(
450450
}
451451

452452
// Factory method for "And" Execution step
453-
absl::StatusOr<std::unique_ptr<ExpressionStep>> CreateAndStep(int64_t expr_id) {
454-
return std::make_unique<LogicalOpStep>(OpType::kAnd, 2, expr_id);
453+
absl::StatusOr<std::unique_ptr<ExpressionStep>> CreateAndStep(size_t num_args,
454+
int64_t expr_id) {
455+
return std::make_unique<LogicalOpStep>(OpType::kAnd, num_args, expr_id);
455456
}
456457

457458
// Factory method for "Or" Execution step
458-
absl::StatusOr<std::unique_ptr<ExpressionStep>> CreateOrStep(int64_t expr_id) {
459-
return std::make_unique<LogicalOpStep>(OpType::kOr, 2, expr_id);
459+
absl::StatusOr<std::unique_ptr<ExpressionStep>> CreateOrStep(size_t num_args,
460+
int64_t expr_id) {
461+
return std::make_unique<LogicalOpStep>(OpType::kOr, num_args, expr_id);
460462
}
461463

462464
// Factory method for recursive logical not "!" Execution step

‎eval/eval/logic_step.h‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,12 @@ std::unique_ptr<DirectExpressionStep> CreateDirectOrStep(
2323
bool shortcircuiting);
2424

2525
// Factory method for "And" Execution step
26-
absl::StatusOr<std::unique_ptr<ExpressionStep>> CreateAndStep(int64_t expr_id);
26+
absl::StatusOr<std::unique_ptr<ExpressionStep>> CreateAndStep(size_t num_args,
27+
int64_t expr_id);
2728

2829
// Factory method for "Or" Execution step
29-
absl::StatusOr<std::unique_ptr<ExpressionStep>> CreateOrStep(int64_t expr_id);
30+
absl::StatusOr<std::unique_ptr<ExpressionStep>> CreateOrStep(size_t num_args,
31+
int64_t expr_id);
3032

3133
// Factory method for recursive logical not "!" Execution step
3234
std::unique_ptr<DirectExpressionStep> CreateDirectNotStep(

‎eval/eval/logic_step_test.cc‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,9 @@ class LogicStepTest : public testing::TestWithParam<bool> {
7373
CEL_ASSIGN_OR_RETURN(step, CreateIdentStep("name1", /*expr_id=*/-1));
7474
path.push_back(std::move(step));
7575

76-
CEL_ASSIGN_OR_RETURN(step, (is_or) ? CreateOrStep(2) : CreateAndStep(2));
76+
CEL_ASSIGN_OR_RETURN(
77+
step, (is_or) ? CreateOrStep(/*num_args=*/2, /*expr_id=*/2)
78+
: CreateAndStep(/*num_args=*/2, /*expr_id=*/2));
7779
path.push_back(std::move(step));
7880

7981
auto dummy_expr = std::make_unique<Expr>();

0 commit comments

Comments
 (0)