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.

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

Using local variable in subroutine through frame pointer #606

Merged
merged 24 commits into from
Dec 23, 2022
Merged

Using local variable in subroutine through frame pointer #606

merged 24 commits into from
Dec 23, 2022

Conversation

ahangsu
Copy link
Contributor

@ahangsu ahangsu commented Nov 18, 2022

closes #559

@ahangsu ahangsu changed the base branch from master to feature/fp-subroutine November 18, 2022 01:42
@ahangsu ahangsu force-pushed the feature/fp-local-var branch 2 times, most recently from 51e3ee0 to 19406de Compare November 21, 2022 16:19
@ahangsu ahangsu changed the title [WIP] Prototype of using local variable in subroutine through frame pointer Prototype of using local variable in subroutine through frame pointer Nov 21, 2022
@ahangsu ahangsu force-pushed the feature/fp-local-var branch from eee8381 to 1f96d76 Compare November 22, 2022 01:00
@ahangsu ahangsu changed the title Prototype of using local variable in subroutine through frame pointer Using local variable in subroutine through frame pointer Nov 22, 2022
@ahangsu ahangsu marked this pull request as ready for review November 22, 2022 20:30
@ahangsu
Copy link
Contributor Author

ahangsu commented Dec 3, 2022

Maybe we want to handle #562 (comment) in this PR

@ahangsu
init commit
bff74b1 
remove popn exposure, seems not useful here

prototyped.

revert test change

minor

update some note
@ahangsu ahangsu force-pushed the feature/fp-local-var branch from a73fcd3 to bff74b1 Compare December 3, 2022 02:40
@ahangsu ahangsu changed the base branch from feature/fp-subroutine to master December 3, 2022 02:41
ahangsu
ahangsu commented Dec 5, 2022
View reviewed changes
pyteal/ast/abi/type.py Outdated Show resolved Hide resolved
tzaffi
tzaffi reviewed Dec 7, 2022
View reviewed changes
Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

Another great improvement in our war against scratch slots. I left a couple of questions.

pyteal/ast/frame.py Outdated Show resolved Hide resolved
pyteal/ast/abi/type.py Show resolved Hide resolved
pyteal/ast/abi/type.py Outdated Show resolved Hide resolved
pyteal/ast/subroutine.py Outdated Show resolved Hide resolved
pyteal/ast/subroutine.py Outdated Show resolved Hide resolved
tzaffi
tzaffi reviewed Dec 8, 2022
View reviewed changes
Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

Upon closer evaluation, I think the original approach using python context-managers is preferred.

Apart from that, there was only one question remaining: how blocked is the PR by #599 ?

pyteal/ast/subroutine.py Outdated Show resolved Hide resolved
tzaffi
tzaffi approved these changes Dec 8, 2022
View reviewed changes
Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

Approving as all my major remarks have been addressed.

@ahangsu ahangsu force-pushed the feature/fp-local-var branch from 4483668 to ff1fc63 Compare December 12, 2022 16:33
@ahangsu ahangsu force-pushed the feature/fp-local-var branch from cda48e8 to 0f1fe4f Compare December 21, 2022 17:27
@jasonpaulos jasonpaulos mentioned this pull request Dec 22, 2022
Merged
@jasonpaulos jasonpaulos mentioned this pull request Dec 23, 2022
Merged
@jasonpaulos jasonpaulos merged commit 90b1e7e into master Dec 23, 2022
@jasonpaulos jasonpaulos deleted the feature/fp-local-var branch December 23, 2022 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaeldiamant michaeldiamant michaeldiamant left review comments

@jasonpaulos jasonpaulos jasonpaulos approved these changes

@tzaffi tzaffi tzaffi approved these changes

@barnjamin barnjamin Awaiting requested review from barnjamin

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

Successfully merging this pull request may close these issues.

Support local variables with frame pointers
4 participants
@ahangsu @tzaffi @michaeldiamant @jasonpaulos