SourceRef in expressions #1357
SourceRef in expressions #1357
Conversation
|
Let's merge the dependency PRs before undrafting this one. |
|
I think once the children function for Expression uses a simple match, we can implement #1169, i.e. remove the Box (and probably also on the whole trait) |
Co-authored-by: chriseth <chris@ethereum.org>
…ti/powdr into source_ref_in_expressions
ast/src/parsed/mod.rs
Outdated
| (Expression::MatchExpression(_, a), Expression::MatchExpression(_, b)) => a == b, | ||
| (Expression::IfExpression(_, a), Expression::IfExpression(_, b)) => a == b, | ||
| (Expression::BlockExpression(_, a), Expression::BlockExpression(_, b)) => a == b, | ||
| _ => false, |
There was a problem hiding this comment.
I think this could be quite dangerous. If we add something to Expression, we will probably not extend this implementation.
Is there a different way of doing this? We probably don't want to clone and set the source ref, but maybe some simple macro?
There was a problem hiding this comment.
I ended up implementing all the comparison inside a macro and using all the variants to make the match exhaustive (without using wildcards), in case we add a new variant to Expressions, the match will complain.
What do you think?
…ed to the enum and is not included
| } | ||
| } | ||
| } | ||
|
|
||
| impl_source_reference!( |
There was a problem hiding this comment.
I trink this is good for now, but i think in general, it would be better to have a macro that allows “iterating” over the elements of Expression.
I.e. it would be much better readable in my opinion if the point of macro call would look like the current macro definition and the macro definition would list the enum variants instead (maybe we can even get that information without listing the names? I saw something like that in some recent code by @leonardoalt ). Then we could reuse the same macro to define the children or even display function of expression
| $( | ||
| (Expression::$variant(_, a), Expression::$variant(_, b)) => a == b, | ||
| )* | ||
| // This catches the case where variants are different and returns false |
| } | ||
| } | ||
| } | ||
|
|
||
| impl_partial_eq_for_expression!( |
There was a problem hiding this comment.
I think the macro I mentioned could be used here as well
This PR solves issue #1293.
Includes PRs that modify some Expression in order to match fields. It is recommended to review/merge those first:
All comments are welcome :)