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

Make minidump-common::format structs writable through scroll. #813

Merged

Conversation

afranchuk
Copy link
Collaborator

As far as I can tell, there was no reason for these structs to not implement Pwrite. This is needed for rust-minidump/minidump-writer#8.

@luser
Copy link
Collaborator

luser commented Feb 23, 2023

Agreed. We simply didn't have any minidump-writing code when I first switched over to using scroll.

I wouldn't block on this, but it might be worth looking at the impact on code size/build time to decide whether it's worth using cargo features to conditionally enable the Pread / Pwrite derives? Presumably minidump-writer doesn't need Pread, and minidump-stackwalk doesn't need Pwrite.

@afranchuk
Copy link
Collaborator Author

afranchuk commented Feb 23, 2023

On my system:

release compile time size
with Pread/Pwrite/SizeWith 7.32s 15MB
without 6.86s 12MB

Averaged over 10 runs.

@gabrielesvelto
Copy link
Collaborator

If crate size is an issue I suppose it's rather easy to add something like:

#[cfg_attr(feature = "write_support", derive(Pwrite)]

I don't know what crate sizes are considered acceptable these days. The increase in compilation time seems very marginal.

@luser
Copy link
Collaborator

luser commented Feb 27, 2023

Given @afranchuk's testing, I don't think it's necessary. If it becomes a problem in the future we can address it.

@gabrielesvelto
Copy link
Collaborator

Alright, merging this

@gabrielesvelto gabrielesvelto merged commit 617c873 into rust-minidump:main Feb 27, 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.

None yet

3 participants