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

Mutable memory #318

Merged
merged 1 commit into from
Aug 2, 2023
Merged

Mutable memory #318

merged 1 commit into from
Aug 2, 2023

Conversation

arcz
Copy link
Collaborator

@arcz arcz commented Jul 18, 2023

Description

Closes #292.

Checklist

  • tested locally
  • added automated tests
  • updated the docs
  • updated the changelog

@arcz arcz marked this pull request as draft July 18, 2023 00:36
@msooseth
Copy link
Collaborator

BTW, the previous failure, :main --quickcheck-replay=116473 -p "buffer-simplification" was reproducible only on buggy Z3. I am thinking about going back to old Z3...

@arcz
Copy link
Collaborator Author

arcz commented Jul 18, 2023

yeah, the tests now fail quite often and I believe this is all caused by the new z3

src/EVM/Types.hs Show resolved Hide resolved
Copy link
Collaborator

@d-xo d-xo left a comment

Choose a reason for hiding this comment

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

Aside from the unfortunate proliferation of type parameters this is surprisingly clean. I had expected integrating St to be more involved tbh. Really nice work <3.

I wonder if it's possible to add some type aliases or newtypes or something to tidy up some of the clutter?

src/EVM.hs Outdated Show resolved Hide resolved
src/EVM.hs Outdated
Comment on lines 1986 to 1991
if srcOffset' >= fromIntegral (BS.length b) then
BS.replicate (fromIntegral size') 0
else
BS.take (fromIntegral size') $
padRight (fromIntegral size') $
BS.drop (fromIntegral srcOffset') b
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we use tryFrom / into here?

src/EVM.hs Outdated Show resolved Hide resolved
@arcz arcz force-pushed the mutable-mem branch 6 times, most recently from 46cf074 to b49c7c5 Compare July 22, 2023 10:27
@arcz arcz force-pushed the mutable-mem branch 3 times, most recently from d4f63b7 to f565d5b Compare July 31, 2023 15:27
@d-xo
Copy link
Collaborator

d-xo commented Aug 2, 2023

What's left to do here? I have a bunch of larger changes I wanna make and I'd like to get this in first to avoid more merge work for you @arcz

@arcz
Copy link
Collaborator Author

arcz commented Aug 2, 2023

I think it's ready, I'm rebasing it on main atm

Copy link
Collaborator

@d-xo d-xo 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 this looks great. No comments. IMO we can merge as soon as CI passes here.

@d-xo d-xo marked this pull request as ready for review August 2, 2023 12:13
@d-xo d-xo merged commit 78ad13b into main Aug 2, 2023
@d-xo d-xo deleted the mutable-mem branch August 2, 2023 13:19
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.

Memory leak with writeWord
3 participants