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

The scope of the unsafe block can be appropriately reduced #3160

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

Conversation

peamaeq
Copy link

@peamaeq peamaeq commented May 3, 2022

Can shrink unsafe blocks

Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR.

Can you please provide some reasoning why this is a better way to handle these unsafe code block? This does not really reduce the scope of anything, it just introduces more blocks with basically the same scope. At least in my opinion thats not a meaningful change.

Comment on lines +116 to 117
let bytes_ptr = self as *const MysqlTime as *const u8;
let bytes = unsafe {
Copy link
Member

Choose a reason for hiding this comment

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

This is one of the relevant change in this PR.

@mohe2015
Copy link
Contributor

mohe2015 commented May 4, 2022

I'm really no expert with unsafe but my understanding is that you should choose the scopes as small as possible and such that any code outside of the unsafe block can't use it's results to cause havoc. Then people only have to look at these small unsafe blocks and as long as they consider them "safe" the whole code should be safe.

@weiznich
Copy link
Member

weiznich commented May 4, 2022

I'm really no expert with unsafe but my understanding is that you should choose the scopes as small as possible and such that any code outside of the unsafe block can't use it's results to cause havoc. Then people only have to look at these small unsafe blocks and as long as they consider them "safe" the whole code should be safe.

This is generally true. The problem with this PR is that it introduces largely changes like the following:

unsafe {
    let error_ptr = PQerrorMessage(conn);
    let bytes = CStr::from_ptr(error_ptr).to_bytes();
    String::from_utf8_lossy(bytes).to_string()
}

is replaced by something like

    let error_ptr = unsafe { PQerrorMessage(conn) };      
    let bytes = unsafe { CStr::from_ptr(error_ptr).to_bytes() };      
    String::from_utf8_lossy(bytes).to_string()   

My main problem with such a change is that is splits up the unsafe block that groups together the ffi call + the type conversion. Both are unsafe and both belong as unit together. In my opinion it's not meaningful to remove the grouping there and to introduce artificial smaller unsafe blocks there. This makes it harder to read + any person trying to review these blocks will need to understand them both at once as they are only meaningful together. So there is just no benefit of having them split up from my point of view.

(Splitting out the String::from_utf8_lossy call would be fine, if that's the only thing this change would do.)

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