erickt / rust-zmq

Rust zeromq bindings.
Apache License 2.0
886 stars 189 forks source link

Remove `owned` flag from `Socket` [nfc] #314

Open kalcutter opened 3 years ago

kalcutter commented 3 years ago

owned is set to false to avoid closing the socket when into_raw called. Set sock to null instead since null is never a valid socket. This simplifies things and reduces the size of the Socket struct.

erickt commented 3 years ago

Looks good, but rather than using a null pointer to signify ownership, what about using Option? That might be a little more appropriate.

kalcutter commented 3 years ago

Using Option<NonNull<<>> would be a bigger change. There are many places we need to get access to the raw pointer. This could be done in different ways:

  1. Use self.sock.map(ptr::NonNull::as_ptr).unwrap_or(ptr::null_mut()) which is quite noisy and kind of defeats the purpose of Option<NonNull<<>> in the first place.
  2. Use self.sock.unwrap().as_ptr()which will generate extra panic code (leading to code bloat). Doing it like this would also be a breaking change because the API allows constructing a socket with from_raw with a null pointer and now most operations on such a socket would panic instead of returning ENOTSOCK.

The current patch is less code than using Option<NonNull<<>>, albeit less explicit. However, I don't think the explicitness is important since it is only used right before drop from into_raw.

I guess I prefer the current patch since it's simpler and more pragmatic.

kalcutter commented 3 years ago

Can we merge this?

kalcutter commented 1 year ago

@Jasper-Bekkers Please consider this change. It simplifies the code and reduces the size of the Socket struct.

Jasper-Bekkers commented 1 year ago

I'm inclined to side with @erickt on this

kalcutter commented 1 year ago

I responded to his comment but never got an answer back.

I am not convinced that using NonNull<> is better in this case as I outlined above. However, even assuming NonNull<> would be better, I think this change is at least an improvement on the status quo and easy to review. As I wrote above, I think using NonNull would be a much larger PR.