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

Remove redundant code #2417

Merged
merged 3 commits into from
Nov 12, 2023
Merged

Conversation

Shaikh-Ubaid
Copy link
Collaborator

@Shaikh-Ubaid Shaikh-Ubaid commented Nov 12, 2023

This PR needs to be rebased after merging of #2414.

Copy link
Collaborator

@Thirumalai-Shaktivel Thirumalai-Shaktivel left a comment

Choose a reason for hiding this comment

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

Otherwise, looks good.

src/libasr/pickle.cpp Outdated Show resolved Hide resolved
src/libasr/pickle.cpp Outdated Show resolved Hide resolved
@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as ready for review November 12, 2023 06:19
@Shaikh-Ubaid Shaikh-Ubaid changed the title Remove redundant pickle code Remove redundant code Nov 12, 2023
It seem LPython and LFortran diverge slightly here. If we update it as per LFortran, then the reference tests change.
@Shaikh-Ubaid
Copy link
Collaborator Author

Shaikh-Ubaid commented Nov 12, 2023

But still, let's fix this the right way. We will keep both checks; if it encounters any of the intrinsic module names, then it is marked as a IntrinsicModule. So I would do this as:

The old approach wasn't a wrong one. We mentioned in comments about divergence so that's like a TODO to be fixed/tackled later. The main goal of this PR is to remove redundant/unused code and therefore I think the reference tests should not change after the removal (atleast not in this PR). If the reference tests change, then it might not be redundant/unused code.

@Shaikh-Ubaid Shaikh-Ubaid merged commit a8dbbc2 into lcompilers:main Nov 12, 2023
13 checks passed
@Shaikh-Ubaid Shaikh-Ubaid deleted the remove_redundant_code branch November 12, 2023 06:52
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