Break/continue expressions #2546
Conversation
|
The code looks reasonable, quick question: is there a motivation for this, or some sort of documentation as to why this is the better way? |
| @@ -1101,6 +1100,9 @@ fn connect_expression( | |||
| } | |||
| Ok(vec![while_loop_exit]) | |||
| } | |||
| // TODO: do we need to create a node here and connect the leaves to it? | |||
canndrew
force-pushed
the
canndrew/break-continue-exprs
branch
from
889b01d
to
f6590bc
Compare
| graph.add_edge(entry_node, break_to_node, "".into()); | ||
| Ok(vec![break_to_node]) | ||
| } | ||
| None => Err(CompileError::BreakOutsideLoop { span }), |
There was a problem hiding this comment.
I suppose this error will be emitted later in ir_generation so that's okay. However, are the graph connections above no longer needed? The idea was to transition the control flow to break_to_node or continue_to_node because everything after a break or continue in a given scope is essentially dead.
There was a problem hiding this comment.
This PR shouldn't change the current behaviour at all since the way break and continue expressions are handled is (if I made no mistakes) exactly the same as if they were enclosed in a single-declaration CodeBlock. I think the problem here is that break_to_node and break_to_continue weren't getting plumbed through to where they were needed anyway, so those variables ended up becoming unused and so I removed them.
canndrew
force-pushed
the
canndrew/break-continue-exprs
branch
from
bd1ba31
to
86da50f
Compare
| @@ -1101,6 +1100,9 @@ fn connect_expression( | |||
| } | |||
| Ok(vec![while_loop_exit]) | |||
| } | |||
| // TODO: do we need to create a node here and connect the leaves to it? | |||
There was a problem hiding this comment.
if there's no node, we can't ever report break or continue as dead code. That's alright, but the proper thing to do here is to connect the current leaves to the break and continue, and then return an empty vec for leaves as a result. That means that the points leading up to the break/continue are connected, and then the path breaks since everything directly after break/continue is actually dead code.
There was a problem hiding this comment.
@sezna: You mean like this?
609f707#diff-8877b4bcb40460d28f3808f38d7efae417662ed07fe5462b224207d08c910130R1104
Note that some of the other cases here which I looked at for guidance seem a bit off. WhileLoop for instance only connects leaves[0] and ignores any others. Array connects it's sub-expressions as if they're all going to be executed in parallel rather than sequentially. I didn't look at any others.
There was a problem hiding this comment.
The purpose of dead code analysis isn't to track execution, but rather usage. So arrays just edge to all of the things they reference so we know that isn't dead code, since the array references them. It doesn't matter if the edges are parallel or not -- just that the item was referenced and is therefore not dead.
There was a problem hiding this comment.
You mean like this?
yup LGTM
canndrew
force-pushed
the
canndrew/break-continue-exprs
branch
from
86da50f
to
04d35b5
Compare
Make
breakandcontinueexpressions rather than declarations in the compiler's internal representation of the AST.