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

Started a rewrite #65

Open
wants to merge 72 commits into
base: master
Choose a base branch
from
Open

Started a rewrite #65

wants to merge 72 commits into from

Conversation

Michael-F-Bryan
Copy link
Owner

@Michael-F-Bryan Michael-F-Bryan commented Jun 30, 2018

(Rendered)
(fixes #64)

@Michael-F-Bryan Michael-F-Bryan mentioned this pull request Jun 30, 2018
17 tasks
@richard-uk1
Copy link

richard-uk1 commented Jul 1, 2018

We also need to update magic-sys's Cargo.toml file to tell cargo about our build script and let it know we link to libmagic (this prevents accidentally linking to two versions of magic-sys because then bad things can happen).

I think if you call your build script build.rs then it 'just works' now.

One could argue that explicitness is better though

@richard-uk1
Copy link

// creation failed. Release MAGIC_CREATED (in drop) and
// return an error
drop(magic);

One of the things I always struggle with is exactly what you need to clean up when, especially when you're in a failure state. I've seen lots of C libs that use goto in this situation, because the default control flow structures just don't cut it. Here is an example. The guide could talk about this/mention that you may need to look in the C source code to know exactly what you have to free when.

@Michael-F-Bryan
Copy link
Owner Author

Michael-F-Bryan commented Jul 1, 2018

One of the things I always struggle with is exactly what you need to clean up when, especially when you're in a failure state.

As a general rule, you'll want to release any resources you acquired during the object's lifetime. In this case the only thing that was acquired was the MAGIC_CREATED "lock", so that's all we need to release.

I've seen lots of C libs that use goto in this situation, because the default control flow structures just don't cut it.

This is true and actually one of the few times where I'd promote the use of goto (assuming there are no better constructs available). A try-catch block is essentially the same as using goto cleanup in that you stop what you're doing and jump straight to the cleanup code. In C++ and Rust we typically use destructors for the same purpose.

EDIT: Ooh, I just found a bug in my code! We shouldn't be using drop() there because it'll also prematurely free libmagic's internal state for the already active instance of Magic.

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.

Guide Rewrite
2 participants