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

Tracking issue for {f32, f64}::copysign #58046

Open
SimonSapin opened this Issue Feb 1, 2019 · 11 comments

Comments

Projects
None yet
8 participants
@SimonSapin
Copy link
Contributor

SimonSapin commented Feb 1, 2019

These methods landed in October 2018 in #55169 without a tracking issue.

impl f32 {
    pub fn copysign(self, y: f32) -> f32 {…}
}
impl f64 {
    pub fn copysign(self, y: f64) -> f64 {…}
}

CC @raphlinus

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Feb 1, 2019

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 1, 2019

Team member @SimonSapin has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@nikic

This comment has been minimized.

Copy link
Contributor

nikic commented Feb 1, 2019

I've mentioned this on the PR before: a.copysign(b) is not very clear on whether the sign of a is applied to b or the sign of b is applied to a. A different name like a.with_sign_of(b) could remove the ambiguity, at the expense of using less standard terminology. (Re-raising this here for visibility, but I think this function is used rarely enough that it ultimately doesn't matter.)

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 1, 2019

@rfcbot concern naming

formally registering @nikic's concern

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Feb 1, 2019

I did see that comment in the PR, but found the next comments convincing:

I'm going to suggest sticking with standard terminology, because I think the target user is much more likely to be studying or adapting numerical algorithms from other languages, rather than than writing Rust-only numerical algorithms. Also I think when studying asm output (especially wasm), it reduces the translation, understanding that copysign in the source should translate into the copysign intrinsic.

I think keeping the standard name copysign makes sense. And you've kept the order of arguments: a.copysign(b) matches copysign(a, b).

@raphlinus

This comment has been minimized.

Copy link
Contributor

raphlinus commented Feb 1, 2019

Sorry for not creating the tracking issue myself, I had it in my list of things to do but didn't get around to it. I don't have much to add; if this were a "de novo" function, then the name change would make sense, but I think consistency with existing usage carries weight.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Feb 2, 2019

This seems fine since it can be replaced with logic that doesn't use an intrinsic if necessary. However, i n future, please include the lang team when exposing intrinsics.

@ZirconiumX

This comment has been minimized.

Copy link

ZirconiumX commented Feb 3, 2019

I think changing the name to be clearer has precedent; we have ptr::copy_nonoverlapping not ptr::memcpy to better explain the invariant, as one example.

I think a.with_sign_of(b) looks good in code, but f32::with_sign_of in documentation looks like a constructor (see Vec::with_capacity for example).

How about a.copy_sign_to(b)/ f32::copy_sign_to?

@raphlinus

This comment has been minimized.

Copy link
Contributor

raphlinus commented Feb 3, 2019

I have two issues with copy_sign_to. First, it seems like the order of arguments is reversed, which I think can be confusing. Second, the "to" makes it sound more like it's mutating.

@ZirconiumX

This comment has been minimized.

Copy link

ZirconiumX commented Feb 4, 2019

@crlf0710

This comment has been minimized.

Copy link
Contributor

crlf0710 commented Feb 4, 2019

copy_sign_from ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment