feat(interpreter): add initial built-in function call path - #100
feat(interpreter): add initial built-in function call path#100Franklin-Qi wants to merge 1 commit into
Conversation
Alonely0
left a comment
Alonely0
left a comment
There was a problem hiding this comment.
I like this quite a bit :)
I have a couple of small comments, other than that I'd ask if you can add some small tests for the fixmes, and annotate them with #[ignore] for the time being. Also, I'll be mostly unavailable until Tuesday or Wednesday, so feel free to take your time and merge this if my review comments are addressed okay.
| pub(crate) fn call<'a>( | ||
| fun: BuiltinFunction, | ||
| args: &[Value<'a>], | ||
| symbols: &SymbolTable<'a>, | ||
| ) -> Result<Value<'a>, BuiltinError> { |
There was a problem hiding this comment.
It is likely we will need to access more properties from the interpreter (like ExecMode) and perform I/O in the future. The latter needs not be addressed for now (tho I'd guess we'll just use Either) , but I think we would benefit from implementing this as a method of the Interpreter. What do you think?
There was a problem hiding this comment.
Yeah, I think that's great.
Lower BuiltinCall to IntrinsicCall carrying BuiltinFunction, dispatch via Registers::get_range(), and implement trivial math/string/bitwise helpers with placeholders for the rest. Also accept builtins in expression context and load ImmF consts. Closes: uutils#96
278c726 to
04f945d
Compare
@Alonely0 Thank you for your review, that's great feedback. I've added some tests. |
2 participants
Lower BuiltinCall to IntrinsicCall carrying BuiltinFunction, dispatch via Registers::get_range(), and implement trivial math/string/bitwise helpers with placeholders for the rest. Also accept builtins in expression context and load ImmF consts.
Closes: #96