Tracking issue for {f32, f64}::copysign

[SimonSapin]

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]

@rfcbot fcp merge

[rfcbot]

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

• @Kimundi
• @SimonSapin
• @alexcrichton
• @dtolnay
• @sfackler
• @withoutboats

Concerns:

• naming resolved by Tracking issue for {f32, f64}::copysign #58046 (comment)

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]

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]

@rfcbot concern naming

formally registering @nikic's concern

[SimonSapin]

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]

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 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.

[Ravenslofty]

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]

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.

[Ravenslofty]

Ah, I typo'd, sorry! I did mean to reverse the arguments and make B mutable. While it may seem arbitrary I think it's clearer, if less composable. I'm not sure if copying the sign of another value is a prime candidate for composition though. I don't think there's any clear way to phrase this without reversing argument order, because "A is acted on by B" sounds clumsy compared to "B acts on A".

…

On Sun, 3 Feb 2019, 21:28 Raph Levien ***@***.*** wrote: 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. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#58046 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABbx29dTo_qNihi-p3I7VGr9Y7IEdUpfks5vJ1SMgaJpZM4ad-b9> .