-
Notifications
You must be signed in to change notification settings - Fork 781
Add predicates for array filtering #8941
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Nice! Thanks for working on it. I notice you went for the rust syntax for closure, but from the discussion in #1328 (comment) , I feel the JS syntax may be a more intuitive syntax. (
I think that's fine for now. A generalisation would be nice, but could be done later. What's important is to test that we get proper error with all the error cases (in the syntax_test) Stuff like the predicate returning the wrong type, or passing a non predicate to one of these functions, or passing a predicate to other function that do not expect a predicate, stuff like This comment is just a preliminary brain dump of some side special case that comes to my mind. |
a79280c to
fc3a325
Compare
09617d3 to
c259f4f
Compare
|
I've updated this to change the syntax to the preferred one and add an interpreter implementation. Let me know if you think there's a better way to accomplish this in the interpreter, as I struggled a bit to do it cleanly. Will add syntax tests + compilation errors for various cases soon. |
…ol predicate return
93862ca to
9800dec
Compare
ogoffart
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delayed answer.
This looks good I think.
Just one comment about the generated code. (C++ and Rust) where the user specified argument could override local variable that needs to be there.
| ?MemberAccess ], | ||
| /// Concatenate the Expressions to make a string (usually expanded from a template string) | ||
| ?MemberAccess, ?Predicate ], | ||
| /// Concatenate the Expressions to make a string (usually expended from a template string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the previous spelling of expanded was correct. (Maybe a merge conflict?)
| } | ||
| }, | ||
| Expression::Predicate { arg_name, expression } => { | ||
| let arg = ident(arg_name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking we probably want to add some kind of prefix here. For example pred_arg_{} so that you don't get mismatch with existing local variables. (and then we need to figure out that the ReadLocalVariable is actually reading a predicate of this name)
Added a predicate type made with rust-style closure syntax (
|x| x == 10). Hardcoded as an 80% solution specifically for use with the array type. To try this out I’ve added array member functions foranyandall, along with an accompanying test.To resolve the predicate argument type, this uses a hardcoded hack on function call resolution. Not sure if this is acceptable, let me know.
The main goal of this is to work towards closing #1328, which will require additional work for inferring a return type on the built in function. But, this also could be added as-is, or with a lowering pass to close #4801.
Implementation here is incomplete (missing interpreter impl) and likely buggy, just pushing now for feedback.