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

Adds from_static_bytes function to Chunk struct #922

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mnmaita
Copy link
Contributor

@mnmaita mnmaita commented Sep 28, 2019

My first contribution here 😄.

This PR tries to address issue #743, I will be drafting this one until I can test it properly.

Suggestions and remarks are welcome! Thanks for creating this repo!

@Cobrand Cobrand marked this pull request as ready for review September 29, 2019 11:54
@Cobrand
Copy link
Member

Cobrand commented Sep 29, 2019

Oopsie, apparently I un-drafted this branch by mistake. For now it looks good to me, ping me whenever you have tested it!

Copy link

@SpacingBat3 SpacingBat3 left a comment

Choose a reason for hiding this comment

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

(Also commenting since I saw this not to be implemented on upstream but made my own implementation as well, it is a bit different and made based on the code of different from_* methods, but I can't say to be better or anything. At least this is pretty much the reason I made this review, hopefully someone will find that useful. Also I can make a PR as well if anyone wants from me to publish my implementation.)

Comment on lines +264 to +271
if raw.is_null() {
Err(get_error())
} else {
Ok(Chunk {
raw: raw,
owned: true,
})
}

Choose a reason for hiding this comment

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

I saw in different from_* methods that there was a private method to just do this stuff to avoid duplicates in the code. Any reason it isn't used there?

EDIT: I guess the code base is out-of-date just by looking at the extended diff… or at least different from the latest stable release. I guess this is also one of the things that should be checked after rebasing the fork and before merging it.

Comment on lines +255 to +257
let rw = unsafe {
sys::SDL_RWFromConstMem(buf.as_ptr() as *const c_void, buf.len() as c_int)
};

Choose a reason for hiding this comment

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

I guess you could use RWops there (exactly RWops::from_bytes(buf)), I've seen other methods to do this and even make it a one-liner in raw declaration (avoiding declaring another var and doing another check I guess). I'm not sure about the benefits with that other than avoiding the use of another unsafe block (I'm not the Rust expert, so I might still be unfamiliar with that language design and quirks, same goes with SDL2).

@mnmaita
Copy link
Contributor Author

mnmaita commented Dec 30, 2023

Hi @SpacingBat3 ! This PR is so old that I barely remember I've ever made it. 😅

Feel free to work on top of it or submit your solution as a new PR! I might not be able to rebase and update this soon enough.

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.

3 participants