Implement override-expression evaluation in functions #5387
Conversation
|
I think the |
|
@teoxoy I've pushed some new commits to the branch. Could you look them over? I'm not sure about "Hoist ConstantEvaluator". It does make the loop body simpler, but maybe it's nice to make it explicit that these |
| // Dummy `emitter` and `block` for the constant evaluator. | ||
| // We can ignore the concept of emitting expressions here since | ||
| // expressions have already been covered by a `Statement::Emit` | ||
| // in the frontend. |
There was a problem hiding this comment.
Super-helpful. At the exact point where I'm thinking, "What in the world?", the comment points out to me what I was missing, and I can just move on.
| for (old_h, mut expr, span) in expressions.drain() { | ||
| if let Expression::Override(h) = expr { | ||
| expr = Expression::Constant(override_map[h.index()]); | ||
| } | ||
| adjust_expr(&adjusted_local_expressions, &mut expr); | ||
| let h = evaluator.try_eval_and_append(expr, span)?; | ||
| debug_assert_eq!(old_h.index(), adjusted_local_expressions.len()); | ||
| adjusted_local_expressions.push(h); | ||
| } |
There was a problem hiding this comment.
The simplicity of this loop is encouraging, as it suggests that changing when constant evaluation occurs would not be such a big change: you'd simply have a loop like this after validation.
There's no need to build a fresh `ConstantEvaluator` for every expression; just build it once and reuse it.
This removes some clones and collects, simplifies call sites, and isn't any more complicated to implement.
I found I needed a little bit more detail here.
jimblandy
force-pushed
the
pipeline-constants
branch
from
e5e625e
to
7f8a56a
Compare
|
Accidentally whacked the upstream branch. |
Properly adjust `AtomicFunction::Exchange::compare` after pipeline constant evaluation.
|
Found some expression handles that weren't getting adjusted. |
jimblandy
force-pushed
the
pipeline-constants
branch
from
8033980
to
28dc70e
Compare
Good catch! We can hopefully avoid those cases once we have some common infrastructure to deal with IR mutation. |
They look good, thanks! |
Tracking meta issue: #4484.