core: reduce code duplication in codegen/builtin_fns #422
No reviewers
Labels
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: M-Labs/nac3#422
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "refactor-builtin-fns"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Used macros to generate some unary math functions.
@ -1048,0 +1053,4 @@
/// Return a constant [`Type`] here if the return type does not depend on the input type.
/// * `on_elem`: The function to be called when the input argument is not an ndarray. Returns [`Option::None`]
/// if the element type & value are faulty and should panic with [`unsupported_type`].
fn helper_call_numpy<'ctx, OnElemFn, RetElemFn, G: CodeGenerator + ?Sized>(
G
into thewhere
clause.call_numpy
makes it seem like you can call any numpy function. Perhaps something likecall_numpy_unaryfunc
?@ -1048,0 +1059,4 @@
(arg_ty, arg_val): (Type, BasicValueEnum<'ctx>),
fn_name: &str,
get_ret_elem_type: &RetElemFn,
on_elem: &OnElemFn,
on_scalar
@ -1070,3 +1113,1 @@
.any(|ty| ctx.unifier.unioned(n_ty, *ty)));
if [ctx.primitives.int32, ctx.primitives.int64]
let fn_name: &str = "abs";
Is this change from
const
tolet
necessary?@ -1079,0 +1156,4 @@
)
}
/// Helper macro. Used so we don't have to keep typing out the type signature of the function.
Documentation comments are to explain what this macro does.
Same for the macros below.
@ -1079,0 +1157,4 @@
}
/// Helper macro. Used so we don't have to keep typing out the type signature of the function.
macro_rules! call_numpy {
I don't think it's appropriate to name the macro
call_numpy
, as it gives the impression that the macro actually invokes the function rather than generates a function which can in turn be used.Same for the macros below.
Revised.
About the macro comments, I have taken the liberty to make changes to the suggested update.
Apart from the nits, the rest looks good to me.
@ -1049,3 +1109,3 @@
generator: &mut G,
ctx: &mut CodeGenContext<'ctx, '_>,
n: (Type, BasicValueEnum<'ctx>),
arg: (Type, BasicValueEnum<'ctx>),
Nit: Please keep this parameter as
n
. I don't think this change is necessary.@ -1079,0 +1192,4 @@
/// * `$fn_name:literal`: To be passed to the `fn_name` parameter of [`helper_call_numpy_unary_elementwise`].
/// * `$on_scalar:expr`: The closure (see below for its type) that acts on float scalar values and returns
/// the boolean results of LLVM type `i1`. The returned `i1` value will be converted into an `i8`.
/// ```rust
Nit: Please add a newline between this and the last line.
Same for the macro below.
Revised.
Please squash the minor documentation/naming changes into their respective commits.
Could we instead make a squash commit? Because I think the history of this branch does not matter too much and might pollute the master branch. The branch also contains commits that revert parts of previous commits.
Sounds good to me.