Rust: Tracking issue for {f32, f64}::copysign

[SimonSapin]

These methods landed in October 2018 in https://github.com/rust-lang/rust/pull/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:

• [x] @Kimundi
• [x] @SimonSapin
• [x] @alexcrichton
• [x] @dtolnay
• [x] @sfackler
• [ ] @withoutboats

Concerns:

• naming resolved by https://github.com/rust-lang/rust/issues/58046#issuecomment-473941682

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 <[email protected] 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
https://github.com/rust-lang/rust/issues/58046#issuecomment-460090222,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABbx29dTo_qNihi-p3I7VGr9Y7IEdUpfks5vJ1SMgaJpZM4ad-b9
.

[crlf0710]

copy_sign_from ?

[joshtriplett]

This seems to have gotten stuck for a while, and it looks like the only blocker is the function name.

Many libraries and developers already describe this function as copysign. While we certainly can change the name if we find it sufficiently unclear or error-prone (such as the example for memcpy, which leads to common problems in C), it seems like people already know this function as copysign. Changing the name would make it harder for people to translate algorithms to Rust or write Rust code given familiarity with other sources of floating-point algorithms.

I would also suggest that we generally seem to do operations this way around, taking the argument and applying it with the operation to self, rather than taking self and applying it with the operation to the argument. Sub::sub takes the argument and subtracts it from self, for instance. So reordering the arguments seems especially error-prone.

I think this is a case where the change will cause more confusion than the established name will.

[nikic]

I looks like the consensus is in favor of copysign(), and I think think the combination of this being both an established concept and a somewhat rarely used function make that a reasonable choice.

Please feel free to mark the concern as resolved (I don't think I can do that myself...)

[SimonSapin]

I think @alexcrichton needs to mark it resolved?

@rfcbot resolve naming

[alexcrichton]

@rfcbot resolve naming

[rfcbot]

:bell: This is now entering its final comment period, as per the review above. :bell:

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

[jethrogb]

I guess I'm a little late with this comment but I think the name should be copy_sign, following Rust naming convention for separating words.

[SimonSapin]

I personally don’t have an opinion on this. (The name without a space has precedent and might help in search engine results.) But since the stabilization PR has merged in the 1.35 cycle, if we rename we should do so this or next week.

[Centril]

My preference is for copy_sign over copysign; seems we could redirect confused users from copysign using some #[rustc_on_misspelled = "copysign"]... iirc @estebank had some idea of that kind?

[jethrogb]

I'm assuming the method name suggestion logic would find copy_sign automatically if someone tries copysign? The documentation search for sure can already do this: https://doc.rust-lang.org/std/?search=copynonoverlapping

[cwhakes]

Maybe too late for bikeshedding, but could the parameter y be renamed to sign? That could alleviate @nikic's concerns. It could cause confusion if someone isn't expecting it to sign to be a f32, but in every case I can think of, the type annotation will be next to the name.

[Centril]

Maybe too late for bikeshedding, but could the parameter y be renamed to sign?

@cwhakes The parameter's name is not part of the stable interface so you can just file a PR for that and I'll review it.

[lzutao]

{f32,f64}::copysign had been stabilized in 1.35.0. Should we close this tracking issue?

[Centril]

Yep.