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

Expose some more functions from libc #25780

Closed
wants to merge 1 commit into from
Closed

Expose some more functions from libc #25780

wants to merge 1 commit into from

Conversation

achanda
Copy link
Contributor

@achanda achanda commented May 25, 2015

#18271

I noticed this issue was moved to the rfc repo after I opened the PR. Should this be closed now and prioritized later?

@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@brson
Copy link
Contributor

brson commented May 25, 2015

r? @aturon

I don't know whether these are desired - some of them have definitely been removed in the past. At the least they need be marked unstable before entering the tree - you can see the unstable attribute syntax elsewhere in std.

@rust-highfive rust-highfive assigned aturon and unassigned brson May 25, 2015
@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 27, 2015
#[unstable(feature = "std_misc",
reason = "unsure about its place in the world")]
#[inline]
pub fn rem(self, other: f32) -> f32 {
Copy link
Member

Choose a reason for hiding this comment

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

I believe that this is already exposed through the Rem trait (LLVM lowers % operations to calls to this function I believe).

@alexcrichton
Copy link
Member

This is an area where we may not want to follow the conventional C names and instead choose a more rustic/descriptive name as well.

@achanda
Copy link
Contributor Author

achanda commented May 27, 2015

Thanks @alexcrichton and @brson . I'm totally open to suggestions on names.

///
/// assert!(abs_diff <= f32::EPSILON)
/// ```
#[unstable(feature = "std_misc",
Copy link
Member

Choose a reason for hiding this comment

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

Could these functions also be introduced under a new feature name? We're trying to cut down on the number of members of the std_misc feature, and something like float_extras may work well here.

@aturon
Copy link
Member

aturon commented May 28, 2015

FWIW, the names of the methods currently available are a bit of a mixed bag. For the most straightforward cases, we tried to hew toward Rust-style names, but for more obscure things like exp_m1 we stuck with the libc names, for familiarity sake. I'm... not really sure what's best for these proposed additions, whose names inherited names are pretty bad! Can we think of something more descriptive?

@achanda
Copy link
Contributor Author

achanda commented May 28, 2015

Thanks all. The doc tests are not passing yet, working on it.

@alexcrichton alexcrichton added the I-needs-decision Issue: In need of a decision. label Jun 2, 2015
@alexcrichton
Copy link
Member

@bors: r+ 02a188b

Thanks @achanda!

@alexcrichton alexcrichton removed the I-needs-decision Issue: In need of a decision. label Jun 10, 2015
@achanda
Copy link
Contributor Author

achanda commented Jun 10, 2015

@alexcrichton the doc tests were not passing when I last tested this on my box. I did not have time to debug this further. Is it okay to merge in this state?

@alexcrichton
Copy link
Member

@bors: r-

Ah no those will need to be fixed before merging.

@achanda
Copy link
Contributor Author

achanda commented Jun 10, 2015

Guessed so. Please give me some time to get back to this.

@alexcrichton
Copy link
Member

We actually had some time to discuss this in triage today and the conclusion was actually to not merge this at this time. The current bindings to libm are unfortunately not the best thought out and it's not clear that we actually want to continue providing them. At this time we think that stemming the flow of new bindings to libm is the best route to take for now while we take some time to work out a concrete story for the functions here. I'll open an associated issue for this, but for now I'm going to close this. Sorry for the delay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants