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

Implemented visit_AsyncFunctionDef in the AST->ASR visitor #2442

Merged
merged 5 commits into from
Dec 14, 2023

Conversation

ujjwaltwitx
Copy link
Contributor

Solves issue no #2312. Implemented visit_AsyncFunctionDef function both in the SymbolTableVisitor and BodyVisitor class.

Comment on lines 4055 to 4062
try
{
// to be implemented
}
catch(const std::exception& e)
{
std::cerr << e.what() << '\n';
}
Copy link
Contributor

@certik certik Dec 12, 2023

Choose a reason for hiding this comment

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

You have to report the error like this:

Suggested change
try
{
// to be implemented
}
catch(const std::exception& e)
{
std::cerr << e.what() << '\n';
}
throw SemanticError("The `async` keyword is currently not supported", x.base.base.loc);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @certik. I will make the necessary changes

@certik certik marked this pull request as draft December 12, 2023 20:59
std::cerr << e.what() << '\n';
}

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of declaring it twice, declare it just once in the base class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this function completely.

@certik
Copy link
Contributor

certik commented Dec 12, 2023

Let's also add a test for this in tests/errors.

@ujjwaltwitx
Copy link
Contributor Author

Let's also add a test for this in tests/errors.

@certik , can you please guide me a bit on how to add the test for the async implementation in tests/errors. I have created the file test_async.py and it contains the below given code

async def test_async():
    print("done")

test_async()

But when I run ./run_tests.py, the file doesn't get executed.

@certik
Copy link
Contributor

certik commented Dec 13, 2023

You have to add it to tests/tests.toml

@ujjwaltwitx ujjwaltwitx marked this pull request as ready for review December 13, 2023 19:11
Comment on lines 4055 to 4062
try
{
// to be implemented
}
catch(const std::exception& e)
{
std::cerr << e.what() << '\n';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove these.

std::cerr << e.what() << '\n';
}
throw SemanticError("The `async` keyword is currently not supported", x.base.base.loc);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Move the function to the CommonVisitor.

@certik certik marked this pull request as draft December 13, 2023 21:15
@certik
Copy link
Contributor

certik commented Dec 13, 2023

Very good, it's almost done. Left couple more comments.

@ujjwaltwitx ujjwaltwitx marked this pull request as ready for review December 14, 2023 07:05
@@ -4051,6 +4056,8 @@ class SymbolTableVisitor : public CommonVisitor<SymbolTableVisitor> {
// Implement visit_Global for Symbol Table visitor.
void visit_Global(const AST::Global_t &/*x*/) {}



Copy link
Contributor

Choose a reason for hiding this comment

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

Remove white space.

@@ -4823,6 +4830,7 @@ class BodyVisitor : public CommonVisitor<BodyVisitor> {
tmp = nullptr;
}


Copy link
Contributor

Choose a reason for hiding this comment

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

remove white space

@certik
Copy link
Contributor

certik commented Dec 14, 2023

It looks good! Last two issues, see my comments above.

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.

This looks great now, thank you!

@certik certik enabled auto-merge (squash) December 14, 2023 16:41
@certik certik merged commit 2afa365 into lcompilers:main Dec 14, 2023
13 checks passed
@ujjwaltwitx ujjwaltwitx deleted the visit_async branch December 14, 2023 17:41
Agent-Hellboy pushed a commit to Agent-Hellboy/lpython that referenced this pull request Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants