macOS unsafe refactor #65
Conversation
| @@ -70,7 +82,7 @@ impl<T> Drop for Dropping<T> { | |||
|
|
|||
| impl<T> Dropping<T> { | |||
| #[inline] | |||
| unsafe fn new(v: *const T) -> Option<Self> { | |||
| fn new(v: *const T) -> Option<Self> { | |||
There was a problem hiding this comment.
This method must remain unsafe. You can use only safe functions to construct an invalid pointer.
There was a problem hiding this comment.
This function appears to be similar to NonNull::new which is safe. This function has the null pointer check that matches it.
Even if the pointer given is invalid, it's only dereferencing it which is unsafe.
There was a problem hiding this comment.
Although I can see your argument that Dropping::new could be needed to be marked unsafe because the type will deterrence the pointer on drop.
| @@ -34,18 +44,20 @@ unsafe fn get_timezone() -> Option<String> { | |||
| let mut buf_bytes = 0; | |||
| let range = CFRange { | |||
| location: 0, | |||
| length: CFStringGetLength(name), | |||
| length: unsafe { CFStringGetLength(name) }, | |||
There was a problem hiding this comment.
+// SAFETY: we tested that `name` is not null, so we know that the result of `CFTimeZoneGetName()` is valid| // New block prevents any variables escaping this scope. | ||
|
|
||
| // If the name is encoded in UTF-8, copy it directly. | ||
| let cstr_ptr = unsafe { CFStringGetCStringPtr(name, kCFStringEncodingUTF8) }; |
There was a problem hiding this comment.
+// SAFETY: we tested that `name` is not null, so we know that the result of `CFTimeZoneGetName()` is valid| let tz = Dropping::new(CFTimeZoneCopySystem())?; | ||
| let name = CFTimeZoneGetName(tz.0); | ||
| let tz = Dropping::new(unsafe { CFTimeZoneCopySystem() })?; | ||
| let name = unsafe { CFTimeZoneGetName(tz.0) }; |
There was a problem hiding this comment.
+// SAFETY: `Dropping::new()` would have returned `None` if the call to `CFTimeZoneCopySystem()` had failed.There was a problem hiding this comment.
👍 to @Kijewski. Let's add safety comments to these unsafe blocks. A safety comment should document how the code is upholding the invariants of the unsafe functions being called.
|
Maybe a lot of things could be made more easy to read if the call of |
|
I refactored the file a lot, and moved all the unsafe function calls into safe methods: https://github.com/Kijewski/iana-time-zone/blob/pr-macos-safe2/src/tz_macos.rs (totally untested). @astraw, should I open a new PR, or do you want to cherry-pick this commit, or do you might want use it as an inspiration to edit your PR? |
Ooo that's nice! I like what you've done there @Kijewski. Please put up a PR. It'd make it easier to compare both approaches, give them reviews, and see what we like better. |
|
I also want to note that if we're interested, we could ask the Servo folks again to merge this change which would allow us to use the higher level bindings again: |
|
I like #67 more than this one. I am closing this and let's move the discussion there. |
As discussed in #64, especially #64 (comment), this refactoring to isolate only the specific calls that need to be unsafe.
I also created a new label Unsafe-Proposed and assigned this PR with it. (Also as discussed in #64).