Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAllow benchmarks to run to completion on macOS #200
Conversation
|
|
| match os_result { | ||
| KERN_SUCCESS => Ok(port), | ||
| KERN_NO_SPACE => Err(MachError::KernNoSpace), | ||
| e => Err(MachError::Unknown(e)), |
This comment has been minimized.
This comment has been minimized.
antrik
May 31, 2018
Contributor
Is there any particular reason why you handle this explicitly, rather than adding the KERN_NO_SPACE case to MachError.from()?
At a glance it looks like that method only handles mach_msg() errors thus far -- but I don't see any reason to keep it that way... Indeed it should probably handle all errors from all kernel calls in use. (Including mach_port_move_member() for example.)
| @@ -505,22 +510,25 @@ impl OsOpaqueIpcChannel { | |||
|
|
|||
| pub struct OsIpcReceiverSet { | |||
| port: Cell<mach_port_t>, | |||
| ports: RefCell<Vec<mach_port_t>>, | |||
This comment has been minimized.
This comment has been minimized.
antrik
May 31, 2018
Contributor
I hope this isn't a dumb question: but why do you need inner mutability here?...
| -1), | ||
| KERN_SUCCESS); | ||
| } | ||
| let error = mach_sys::mach_port_mod_refs(mach_task_self(), |
This comment has been minimized.
This comment has been minimized.
antrik
May 31, 2018
Contributor
Is there any reason for using a different syntax for handling the error here than for the same call above?...
(BTW, as the mach_port_mod_refs() call is needed in several places, I'd say it would be useful to have a helper function for that...)
| @@ -428,3 +428,11 @@ fn test_reentrant() { | |||
| sender.send(null.clone()).unwrap(); | |||
| assert_eq!(null, receiver.recv().unwrap()); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn transfer_closed_sender() { | |||
This comment has been minimized.
This comment has been minimized.
antrik
May 31, 2018
Contributor
This is a bit nit-picky: but would you mind adding the test in a separate commit after the fix?
When making this sort of decisions, I like thinking in terms of both bisectability and revertability. Bisectability tells us that the test needs to go either in the same commit as the fix, or after it, since otherwise we would have a known-broken state in the history. Revertability says that it's better to have the test separate (either before or after the fix), since the test is probably valid regardless of the fix -- so if someone ever tries to back out the fix for any reason, the test should be still there and fail, thus making it clear that the issue needs to be addressed in some way... Considering both bisectability and revertability, we can conclude that a separate commit after the fix is indeed the best variant.
(Just to be clear, there are of course other situations as well: for tests that check newly added functionality, the same analysis would conclude that the tests need to go into the same commit as the change adding the functionality in question; and for tests that guard against regressions in things that should work even before the change, the test should go into a separate commit before the change...)
|
I've addressed all of the comments. MachError::from can't easily handle the kernel errors because kernel_return_t and mach_msg_return_t are just aliases of the same type, and the range of errors that MachError::from handles overlaps with the range of the kernel return values. |
|
I know about the type aliasing -- but I don't understand why it is a problem? Isn't the whole idea behind this to allow uniform handling of all the errors?... Doesn't really matter though, since I like the solution you implemented :-) All the changes look very good to me. However, as I have limited experience with the |
|
Looks good to me. |
|
@bors-servo r=pcwalton |
|
|
|
|
jdm commentedMay 30, 2018
These commits fix a series of panics that I encountered while attempting to run benchmarks on macOS. The benchmarks now complete, as can be observed in #199.