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

feat: add Stack empty #876

Closed
wants to merge 1 commit into from
Closed

Conversation

mattsse
Copy link
Collaborator

@mattsse mattsse commented Nov 21, 2023

Adds a helper function to get an empty Stack.

this can be beneficial during tracing where stack capturing is optional, and working with Stack over Option<Stack> is preferred

ref paradigmxyz/reth#5521

@rakita
Copy link
Member

rakita commented Nov 22, 2023

cc #816 as this stack is going to be overwritten.

Copy link
Member

@rakita rakita left a comment

Choose a reason for hiding this comment

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

Wait, this would make functions invalid in Stack as they depend on STACK_LIMIT, so best not to include this.

@rakita
Copy link
Member

rakita commented Nov 22, 2023

Additionally, I think you should just clone the stack slice as they have separate purpose that don't overlap, and when a shared stack comes this is going to be needed.

@mattsse
Copy link
Collaborator Author

mattsse commented Nov 22, 2023

Wait, this would make functions invalid in Stack as they depend on STACK_LIMIT, so best not to include this.

ah because the memory operations would be invalid, like ptr based copies? which require capacity?

makes sense, but then this type should not be Clone

@gbrew
Copy link

gbrew commented Nov 22, 2023

Wait, this would make functions invalid in Stack as they depend on STACK_LIMIT, so best not to include this.

ah because the memory operations would be invalid, like ptr based copies? which require capacity?

makes sense, but then this type should not be Clone

Yeah, I was just looking at this, and I agree the #[derive(Clone)] on Stack is unsafe, as it won't preserve the invariant that the stack is fully allocated.

@mattsse mattsse mentioned this pull request Nov 22, 2023
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.

4 participants