Skip to content

Use same type for all CLONE_ flags. #147

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

Merged
merged 1 commit into from
Jan 23, 2016
Merged

Use same type for all CLONE_ flags. #147

merged 1 commit into from
Jan 23, 2016

Conversation

fiveop
Copy link
Contributor

@fiveop fiveop commented Jan 21, 2016

The CLONE_ flags defined in ...notbsd/mod.rs and ...notbsd/linux/mod.rs use different types (c_int and c_uint respectively). This commit changes the type of the latter to c_int as well, since the implementation of clone of glibc takes an int (see man 2 clone).

Is there a reason, why not all of the flags are in ...notbsd/mod.rs?

@brson
Copy link
Contributor

brson commented Jan 21, 2016

Looks like this will be a breaking change.

@alexcrichton
Copy link
Member

@brson would you be ok merging this despite that? I'd be very willing to say that this will break no code because:

  • These types accidentally disagree with the other constants in src/unix/notbsd/mod.rs
  • If you were to call the function with these flags, you'd get a type error

Which means that any current usage has a cast, or they're not being used. Along those lines I would rather make this change and push a new version. If it does indeed cause breakage we can yank and republish and figure out another strategy, but I'm not sure it'll be worth keeping around this "wart" for too long.

@brson brson added the relnotes label Jan 21, 2016
@brson
Copy link
Contributor

brson commented Jan 21, 2016

@alexcrichton I think we should decide between the libs team what to do about this and the eventfd patch. I don't want to start a habit of publishing minor breakage casually.

@alexcrichton
Copy link
Member

Perhaps. I worry that it will be at least two weeks before we can triage this at which point it may be "even more too late". If you feel strongly then we can just reject these patches and say "oh well". I feel very strongly that a major version bump isn't worth it, and PRs like this are just making it more useful.

I wish we could typecheck constants but unfortunately we can't, so this is just a vector via which we're bound to make mistakes. I can't really envision a realistic scenario in which this causes breakage, however.

@fiveop
Copy link
Contributor Author

fiveop commented Jan 22, 2016

For what it's worth, I'd rather have a correct library, than the luxury of no breakage between versions (and code like let flags = A as c_int | B;).

I found this inconsistency because I was trying to convert the corresponding constants in nix-rust to a bitflags! type. Anyone who used the clone function (or any of its siblings) with the wrongly typed constants would have had to cast them.

I'll continue creating pull request in cases such as these and eagerly await your decision.

@brson
Copy link
Contributor

brson commented Jan 22, 2016

@alexcrichton Ok, let's merge this and #126. I agree in practice it's not going to break anything so there's no practical reason to hold it up. I'll think about how to incorporate this into our regression tracking.

@alexcrichton
Copy link
Member

Ok, I'm going to merge this with #149 so we don't have to rebase all these PRs.

@alexcrichton alexcrichton merged commit a7ce06c into rust-lang:master Jan 23, 2016
@fiveop fiveop deleted the clone_common_type branch February 28, 2016 15:12
danielverkamp pushed a commit to danielverkamp/libc that referenced this pull request Apr 28, 2020
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