FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Store initializer exprs for Structs in ASR by Smit-create · Pull Request #2056 · lcompilers/lpython · GitHub

/ lpython Public

Store initializer exprs for Structs in ASR - #2056

Merged
certik merged 8 commits into
lcompilers:mainfrom
Smit-create:i-1981-1
Jun 30, 2023
ConversationCommits (8)ChecksFiles changed
Merged

Store initializer exprs for Structs in ASR#2056
certik merged 8 commits into
lcompilers:mainfrom
Smit-create:i-1981-1

Conversation

Smit-create commented Jun 29, 2023

Copy link
Copy Markdown
Collaborator

No description provided.

Smit-create commented Jun 29, 2023

Copy link
Copy Markdown
Collaborator Author

@certik This is an alternative approach to #2013 and I think this is more robust and doesn't create any confusion.

certik commented Jun 29, 2023

Copy link
Copy Markdown
Contributor

I think that this is fine. Isn't it equivalent to store an iniitalizer in the Variable (inside a struct) or in the Struct?

Smit-create commented Jun 30, 2023

Copy link
Copy Markdown
Collaborator Author

I think that this is fine. Isn't it equivalent to store an iniitalizer in the Variable (inside a struct) or in the Struct?

Yeah, that works. But it would require a lot of methods to be moved in CommonVisitor and may create problems of visiting them again in BodyVisitor which may affect the performance.

Smit-create mentioned this pull request Jun 30, 2023
Closed
Smit-create requested a review from certik June 30, 2023 06:44
Smit-create marked this pull request as ready for review June 30, 2023 06:44

Smit-create commented Jun 30, 2023

Copy link
Copy Markdown
Collaborator Author

@certik This PR fixes all the issues related to #1981, and #2013. Moreover, also fixes the test in #2058.

certik approved these changes Jun 30, 2023
View reviewed changes

certik left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ok. I think this PR is reasonably clean and doesn't require major refactor. So let's merge it. I think this still keeps the door open to refactor things later if we figure out a better design.

certik merged commit e642072 into lcompilers:main Jun 30, 2023
Smit-create deleted the i-1981-1 branch June 30, 2023 15:16
Smit-create mentioned this pull request Jun 30, 2023
Closed
Smit-create mentioned this pull request Jul 31, 2023
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

certik certik approved these changes
czgdp1807 czgdp1807 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL