move InList related simplify to one place #9037
Conversation
| impl TreeNodeRewriter for InListSimplifier { | ||
| type N = Expr; | ||
|
|
||
| fn mutate(&mut self, expr: Expr) -> Result<Expr> { |
There was a problem hiding this comment.
I wonder what the the reason to have multiple different simplifiers? As written now, each TreeNodeRewriter makes a different pass over the expression tree. I think we could make a single pass.
For example, could we have a single function in this module like `fn simplify(expr: Expr) -> Result```
That was called as part of Expr::Simplifier::mutate? I think that could make the code simpler as well as more performant
|
Hi @guojidan -- I wonder if you are still working on this PR? What do you think about moving all the rewrites into the same pass? |
all rewrites means all |
Yes, sorry this is what i meant -- all the InList code |
There was a problem hiding this comment.
Thank you @guojidan -- this looks good to me. I think as a follow on PR it would be nice to consolidate all the InList rewrites into a single pass (rather than ShortenInListSimplifier and InListSimplifier but this PR seems like a good step in that direction
Thanks again
|
In my opinion we could merge this PR or if you prefer you could consolidate the passes into a single Struct as part of this PR as well. Let me know what you you prefer. |
Hi @alamb , consolidate |
|
Thanks again @guojidan ! |
Which issue does this PR close?
Part of #8970.
Rationale for this change
move all InList related simplify to one place
Are these changes tested?
no
Are there any user-facing changes?
no