Implement override-expression evaluation in functions#5387
Implement override-expression evaluation in functions#5387teoxoy merged 12 commits intogfx-rs:pipeline-constantsfrom
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.
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. |
8033980 to
28dc70e
Compare
jimblandy
left a comment
There was a problem hiding this comment.
Okay! Please review my commits before merging, but with those fixes, this PR looks good to me.
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.