Conversation
iangneal
left a comment
iangneal
left a comment
There was a problem hiding this comment.
Overall looks fine, though I am missing some context for this project. Where are the end-to-end tests for the IR usage? Overall this is a lot of code and feels a bit under tested.
ir/src/expr/aexpr.rs
Outdated
| *self = inner.clone(); | ||
| } | ||
| } | ||
| IRAexprImpl::Sum(_, _) => todo!(), |
There was a problem hiding this comment.
Is this for another PR? Or is there an issue attached to these todos?
There was a problem hiding this comment.
No, since I've been working on and off at this for a while I basically forgot
| } | ||
| IRBexprImpl::And(exprs) => { | ||
| for expr in exprs { | ||
| expr.canonicalize(); |
There was a problem hiding this comment.
Maybe I misunderstand, but this canonicalizes the inner expressions without then seeing if they can be merged at the top level; for example, (&& (&& a b) (&& c d)) should be canonicalized (I think) to (&& a b c d), but it doesn't appear that this happens here. Same with the OR case.
There was a problem hiding this comment.
Because that idea didn't occur to me... but makes sense
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Do we have any tests for all the canonicalization logic?
|
|
||
| /// Folds the statements if the expressions are constant. | ||
| /// If a assert-like statement folds into a tautology (i.e. `(= 0 0 )`) gets removed. If it | ||
| /// folds into a unsatisfiable proposition the method returns an error. |
There was a problem hiding this comment.
This comment doesn't seem particularly relevant here.
This is so low priority that I don't blame you if you don't remember what all this was about. This crate defines the IR used by the lowering pipeline: Is in a separate crate from the rest of the IR generation because clients may need to emit custom IR, so this keeps the dependencies simpler and a bit lighter. The rest of the framework currently lives here and my plan is to incrementally merge into
The IR has been tested while using it... but you are right that the crate on it's own is under tested. I'll work on that. |
2 participants
Final version of the IR crate