Support floor division as intrinsic function #2372
Conversation
So it can be used in intrinsic_function_registry
Shaikh-Ubaid
force-pushed
the
intrinsic_floor_div
branch
2 times, most recently
from
b247454
to
4b917cf
Compare
Shaikh-Ubaid
force-pushed
the
intrinsic_floor_div
branch
from
8e80b97
to
235117b
Compare
|
Ready. |
|
|
||
| ASR::expr_t* perform_casting(ASR::expr_t* expr, ASR::ttype_t* src, | ||
| ASR::ttype_t* dest, Allocator& al, | ||
| const Location& loc); |
There was a problem hiding this comment.
What is the idea behind these? I think we do not want to do any kind of automatic casting in ASR, nor be responsible for determining what proper casting must be done.
There was a problem hiding this comment.
I think we do not want to do any kind of automatic casting in ASR
It is actually not for automatic casting. The floordiv intrinsic we defined in this PR is generalized so that it can work on any type (from the supported types of int, unsigned int, real, logical). The casting function is used to cast the intermediate temporary variable to the appropriate return type.
There was a problem hiding this comment.
It is also used to cast the function arguments to the needed intermediate type.
There was a problem hiding this comment.
Is this a helper function to make make_Cast_t_value more convenient, but you still tell the initial and final type explicitly?
There was a problem hiding this comment.
Is this a helper function to make make_Cast_t_value more convenient, but you still tell the initial and final type explicitly?
Yes. The way it helps is we need not worry about what type it currently is and just specify what type we want it to be.
|
In general the change is good I think, but I want to have good clarity on the casting situation. We should document this in comments, once I understand it. |
Sure, I clarified your queries above. |
|
Please let me know if you have further doubts in the implementation. I will be happy to clarify them. |
|
Thank you so much for the review.
Sure. Is it fine to merge this PR currently? |
|
Yes, you can merge it. |
fixes #2349