Skip to content

Convert initialisation at declaration to assignments and fix Variable.value and Variable.m_symbolic_value settings #662

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 10 commits into from
Jun 21, 2022
Merged

Convert initialisation at declaration to assignments and fix Variable.value and Variable.m_symbolic_value settings #662

merged 10 commits into from
Jun 21, 2022

Conversation

czgdp1807
Copy link
Collaborator

@czgdp1807 czgdp1807 commented Jun 21, 2022 •
edited
Loading

Closes #661
#651

@czgdp1807 czgdp1807 added the asr ASR related changes label Jun 21, 2022
@czgdp1807 czgdp1807 mentioned this pull request Jun 21, 2022
Closed
certik
certik approved these changes Jun 21, 2022
View reviewed changes
Copy link
Contributor

@certik certik left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think that this is fine.

The global variables in Python are effectively just like local (and should be handled like local): see #649 for an example. But let's fix it one step at a time. First inside functions, only later let's tackle the code at the global level.

@czgdp1807
Copy link
Collaborator Author

czgdp1807 commented Jun 21, 2022 •
edited
Loading

The issue is not with global and local variables. Its related to incorrect setting of value attribute of Variable. The Variable node is same for both global and local variables. Therefore it doesn’t matter. There are only two possible situations, either it is fixed completely or it is not fixed at all. In any case, I think its almost done. If I feel that the fix is getting messy then I will split it. Will let you know shortly. 😊.

@czgdp1807 czgdp1807 marked this pull request as ready for review June 21, 2022 13:14
@czgdp1807 czgdp1807 added the llvm LLVM related changes label Jun 21, 2022
@czgdp1807
Copy link
Collaborator Author

@certik This is ready. I am going to keep this PR for the fixes (as they are clean and simple IMO). Please let me know if any change is required. Thanks.

@czgdp1807
Copy link
Collaborator Author

Tests pass.

@certik certik merged commit f49908e into lcompilers:main Jun 21, 2022
@certik
Copy link
Contributor

certik commented Jun 21, 2022

Looks good, thanks!

@czgdp1807 czgdp1807 deleted the init_decl1 branch June 21, 2022 13:56
@czgdp1807
Copy link
Collaborator Author

Great. Fixing #667 right now. Made some comments on your PR, #666

@czgdp1807 czgdp1807 mentioned this pull request Sep 24, 2022
Closed
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

Assignees
No one assigned
Labels
asr ASR related changes llvm LLVM related changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect results due to outdated compile time values
2 participants
@czgdp1807 @certik