-
-
Notifications
You must be signed in to change notification settings - Fork 129
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
fix: Kind for CMPLX and ArrayConstructor doesn't work #3670
Conversation
@@ -5129,6 +5129,9 @@ class CommonVisitor : public AST::BaseVisitor<Derived> { | |||
Vec<ASR::expr_t*> args; args.reserve(al, 1); | |||
args.push_back(al, arg); | |||
array->m_args[i] = ASRUtils::expr_value(ASRUtils::EXPR(create_func(al, loc, args, diag))); | |||
ASR::Array_t* arr_type = ASR::down_cast<ASR::Array_t>(array->m_type); |
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.
Here, it seems like kind
is being treated as an elemental intrinsic function.
While in the documentation, kind
is actually an "Inquiry function" (screenshot from page 384, J3/18-007r1):
.
So here, kind([real :: 1, 2, 3, 4])
would act on the array element-wise, which isn't what kind
is supposed to do though.
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.
Okay then this will be a design change, that can be done in another PR?
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.
Okay then this will be a design change, that can be done in another PR?
That sounds like a good way. Can I re-think implications of the added code on other element-wise intrinsic functions?
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.
Yes, we just need to find an elemental intrinsic that takes real
and returns integer
.
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 thought of floor
and ceiling
, but seems like they raise an error on array constructor input (a bug).
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 created a bug report here: #3673 for issues with floor
, ceiling
etc.
I do agree that these changes look good, I'm happy with it.
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.
Thank you for that, I'll check why it fails and try to fix it.
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.
@Pranavchiku , don't worry, I'm already working on that.
I think the above solution works for |
The example maybe wrong, we can put |
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 happy with the changes, if you could ensure that it doesn't actually close the issue #3601, as the constructor issue isn't handled in this.
@@ -5129,6 +5129,9 @@ class CommonVisitor : public AST::BaseVisitor<Derived> { | |||
Vec<ASR::expr_t*> args; args.reserve(al, 1); | |||
args.push_back(al, arg); | |||
array->m_args[i] = ASRUtils::expr_value(ASRUtils::EXPR(create_func(al, loc, args, diag))); | |||
ASR::Array_t* arr_type = ASR::down_cast<ASR::Array_t>(array->m_type); |
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 created a bug report here: #3673 for issues with floor
, ceiling
etc.
I do agree that these changes look good, I'm happy with it.
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 this PR is fine. @Pranavchiku if you are ok with it, you can merge it.
Fixes #3601