Skip to content
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 use of keyword args for intrinsic functions #3075

Merged
merged 7 commits into from
Jan 14, 2024

Conversation

HarshitaKalani
Copy link
Contributor

@HarshitaKalani HarshitaKalani commented Jan 10, 2024

Fixes: #3053
Towards: #3052

With this PR, I'm handling the presence of keywords with the args in intrinsic functions - #3053 (comment)
This should help in resolving the issue while building stdlib, however there are still changes that need to be done.

@certik
Copy link
Contributor

certik commented Jan 10, 2024

Thanks for working on this, very helpful.

@Pranavchiku Pranavchiku added the stdlib Issues related to compiling fortran-lang/stdlib label Jan 11, 2024
@HarshitaKalani HarshitaKalani marked this pull request as ready for review January 11, 2024 17:51
@@ -4196,7 +4196,7 @@ class CommonVisitor : public AST::BaseVisitor<Derived> {

ASR::asr_t* create_ArraySize(const AST::FuncCallOrArray_t& x) {
Vec<ASR::expr_t*> args;
std::vector<std::string> kwarg_names = {"dim", "kind"};
std::vector<std::string> kwarg_names = {"array", "dim", "kind"};
handle_intrinsic_node_args(x, args, kwarg_names, 1, 3, std::string("size"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's separate these changes into another PR. But would modify the handle_intrinsic_node_args function to rename kwarg_names to just arg_names which would list all arguments, and then the min/max arg number would still be 1, 3 in this case, and everything should work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even later we can then refactor these to use the model in #3090.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example the function handle_intrinsic_node_args can just access the name2signature dictionary to access the information.

@certik certik marked this pull request as draft January 12, 2024 16:04
@certik
Copy link
Contributor

certik commented Jan 12, 2024

I made this a draft for now. Once you address #3075 (comment) then make it ready for review again.

@HarshitaKalani
Copy link
Contributor Author

@certik this is ready. Can you please review it?

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good, thanks!

@certik certik merged commit 3d979c6 into lfortran:main Jan 14, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Issues related to compiling fortran-lang/stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement intrinsic merge
3 participants