Skip to content

New string Method endswith() support #1606

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 23, 2023
Merged

New string Method endswith() support #1606

merged 2 commits into from
Mar 23, 2023

Conversation

harshsingh-24
Copy link
Contributor

@harshsingh-24 harshsingh-24 commented Mar 22, 2023 •
edited
Loading

  • Added str.endswith(suffix) functionality which returns True if the string ends with the specified suffix, otherwise return False
  • Added test cases which covers both Statement and Branch Coverage associated with endswith() functionality.
  • Added comments for developer understanding

Can you please review? @certik @Thirumalai-Shaktivel @Smit-create @czgdp1807

@Smit-create
Copy link
Collaborator

CI fails because of this:

/home/runner/work/lpython/lpython/integration_tests/_lpython-tmp-test-c/test_str_attributes.c:3:1: error: expected identifier or ‘(’ before ‘%’ token
    3 | %
      | ^
[ 91%] Built target test_platform
/home/runner/work/lpython/lpython/integration_tests/_lpython-tmp-test-c/test_str_attributes.c:4:8: error: stray ‘#’ in program
    4 | a1234PT#$

That's an error generated from the C backend, will look into it.

@harshsingh-24
Copy link
Contributor Author

CI fails because of this:

/home/runner/work/lpython/lpython/integration_tests/_lpython-tmp-test-c/test_str_attributes.c:3:1: error: expected identifier or ‘(’ before ‘%’ token
    3 | %
      | ^
[ 91%] Built target test_platform
/home/runner/work/lpython/lpython/integration_tests/_lpython-tmp-test-c/test_str_attributes.c:4:8: error: stray ‘#’ in program
    4 | a1234PT#$

That's an error generated from the C backend, will look into it.

Hmm. Quite weird though. That is one of my test cases which I added for integration test. It says a stray # but that is present inside a string.

czgdp1807
czgdp1807 reviewed Mar 22, 2023
View reviewed changes
Copy link
Collaborator

@czgdp1807 czgdp1807 left a comment

Choose a reason for hiding this comment

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

Looks fine to me. @Smit-create Please fix the CI and then I will approve after reviewing it for the second time.

@czgdp1807 czgdp1807 marked this pull request as draft March 22, 2023 13:17
@certik
Copy link
Contributor

certik commented Mar 22, 2023

This is fine for now, eventually this should probably be an IntrinsicFunction as well.

Thirumalai-Shaktivel
Thirumalai-Shaktivel reviewed Mar 23, 2023
View reviewed changes
@harshsingh-24
Copy link
Contributor Author

harshsingh-24 commented Mar 23, 2023 •
edited
Loading

This is fine for now, eventually this should probably be an IntrinsicFunction as well.

By Intrinsic Function you mean like how ListInsert, DictPop have been implemented in LPython? How can I implement it?

@harshsingh-24 harshsingh-24 marked this pull request as ready for review March 23, 2023 06:19
@harshsingh-24
Copy link
Contributor Author

Ready for review @czgdp1807 and @Smit-create

@harshsingh-24 harshsingh-24 requested review from Smit-create and removed request for Thirumalai-Shaktivel March 23, 2023 08:07
@harshsingh-24
Copy link
Contributor Author

If done reviewing, can you please merge these changes? @czgdp1807 @certik

certik
certik reviewed Mar 23, 2023
View reviewed changes
src/lpython/semantics/python_ast_to_asr.cpp
res = std::equal(suffix.rbegin(), suffix.rend(), s_var.rbegin());

tmp = ASR::make_LogicalConstant_t(al, loc, res,
ASRUtils::TYPE(ASR::make_Logical_t(al, loc, 4, nullptr, 0)));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is doing a compile time optimization. I think this should rather be implemented in the optimizer, but not done by the frontend, since ASR is then being lowered, and when we convert ASR back to Python, the original code is not recovered.

Copy link
Contributor Author

@harshsingh-24 harshsingh-24 Mar 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This should be refactored into IntrinsicFunction. Just add a new function. We will have thousands of such functions eventually.

@certik How can I go on with refactoring of code to Intrinsic Functions? Can you give a head start? Because what I understood from @Thirumalai-Shaktivel is that we have to sync between LPython and LFortran for that. Can you give some pointers here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we have to sync first. Until then, you can implement this in LFortran itself, in libasr.

Copy link
Contributor Author

@harshsingh-24 harshsingh-24 Mar 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Where in libasr? Can you give an example of one such addition in LFortran? Maybe a previous PR or something. @certik

Copy link
Contributor

Choose a reason for hiding this comment

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

in src/libasr.

Just look into merged PRs and search IntrinsicFunction. See also #1616 that is porting it to LPython.

certik
certik approved these changes Mar 23, 2023
View reviewed changes
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 it's fine. It adds technical debt, since I think we really have to handle these things with the IntrinsicFunction approach and handle all optimizations in the optimizer and simplify the frontend. And then reuse this in LFortran as well (if desired).

I am going to merge it, as I think generally it is done well, but we need to focus on the proper design from now on, to reduce technical debt going forward.

@certik certik merged commit dd5a8f9 into lcompilers:main Mar 23, 2023
@certik
Copy link
Contributor

certik commented Mar 23, 2023

This should be refactored into IntrinsicFunction. Just add a new function. We will have thousands of such functions eventually.

@anutosh491 anutosh491 mentioned this pull request Mar 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@certik certik certik approved these changes

@Thirumalai-Shaktivel Thirumalai-Shaktivel Thirumalai-Shaktivel left review comments

@czgdp1807 czgdp1807 czgdp1807 approved these changes

@Smit-create Smit-create Awaiting requested review from Smit-create

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@harshsingh-24 @Smit-create @certik @czgdp1807 @Thirumalai-Shaktivel