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

unistd: Redesign the enum returned by fork() #332

Merged
merged 2 commits into from
Mar 31, 2016

Conversation

kamalmarhubi
Copy link
Member

This changes the name of the enum returned by fork() to ForkResult,
and changes the Parent variant to be struct-like.

The result can be matched like

use nix::unistd::ForkResult::*;
match fork().unwrap() {
    Parent { child } => { ... }
    Child => { ... }
}

using the shorthand matching syntax for struct-like enum variants.

This is a breaking change.

@kamalmarhubi
Copy link
Member Author

Review note: I'd like more than one person to comment on this, and to figure out if this should be made a convention.

I can't immediately think of anywhere eles in nix that we do something similar to fork. Anyone have any handy?

@@ -27,7 +29,7 @@ impl Fork {

pub fn is_parent(&self) -> bool {
match *self {
Fork::Parent(_) => true,
Fork::Parent {..} => true,
_ => false
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

pub fn is_parent(&self) -> bool {
    !is_child(self)
}

while you're at it? :)

@fiveop
Copy link
Contributor

fiveop commented Mar 30, 2016

I could not guess the intent of the struct before looking how it was used (as fork() result). Why don't we call it ForkResult? It does not represent a fork, it represents the result of a fork.

@homu
Copy link
Contributor

homu commented Mar 30, 2016

☔ The latest upstream changes (presumably #336) made this pull request unmergeable. Please resolve the merge conflicts.

@kamalmarhubi
Copy link
Member Author

I'll do that once I get back to the computer.

This commit changes the name of the enum returned by `fork()` to
`ForkResult`, and changes the `Parent` variant to be struct-like.

The result can be matched like

    use nix::unistd::ForkResult::*;
    match fork().unwrap() {
        Parent { child } => { ... }
        Child => { ... }
    }

using the shorthand matching syntax for struct-like enum variants.

This is a breaking change.
@kamalmarhubi kamalmarhubi changed the title fork: Make Parent a struct-like variant unistd: Redesign the enum returned by fork() Mar 30, 2016
@kamalmarhubi
Copy link
Member Author

@fiveop done!

@posborne
Copy link
Member

Looks good to me. I can't think of other similar calls off hand, but it wouldn't surprise me if one exists. I don't think we support it at present, but forkpty would certainly look the same (as it calls fork()).

@kamalmarhubi
Copy link
Member Author

Not yet, but soon: rust-lang/libc#246

@fiveop
Copy link
Contributor

fiveop commented Mar 31, 2016

@homu r+

What lead you to mark our fork wrapper as inline? I do not disagree. I think it makes sence for our wrapper functions that only add error wrapping (I don't want to call it handling). Maybe this is another thing for our conventions?

@homu
Copy link
Contributor

homu commented Mar 31, 2016

📌 Commit c2f8bb7 has been approved by fiveop

@homu
Copy link
Contributor

homu commented Mar 31, 2016

⌛ Testing commit c2f8bb7 with merge 4ca407d...

homu added a commit that referenced this pull request Mar 31, 2016
unistd: Redesign the enum returned by fork()

This changes the name of the enum returned by `fork()` to `ForkResult`,
and changes the `Parent` variant to be struct-like.

The result can be matched like

    use nix::unistd::ForkResult::*;
    match fork().unwrap() {
        Parent { child } => { ... }
        Child => { ... }
    }

using the shorthand matching syntax for struct-like enum variants.

This is a breaking change.
@homu
Copy link
Contributor

homu commented Mar 31, 2016

☀️ Test successful - status

@homu homu merged commit c2f8bb7 into nix-rust:master Mar 31, 2016
@kamalmarhubi
Copy link
Member Author

What lead you to mark our fork wrapper as inline? I do not disagree. I think it makes sence for our wrapper functions that only add error wrapping (I don't want to call it handling).

Vaguely, I think I'd been looking at something in std and saw some things that were slightly more complex than wrappers marked as #[inline]. The #[inline] attribute is only a suggestion, anyway.

Maybe this is another thing for our conventions?

This overall is an interesting point. Opened #338 to discuss.

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.

4 participants