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

Updates to the Float API #13597

Merged
merged 4 commits into from
Apr 23, 2014
Merged

Updates to the Float API #13597

merged 4 commits into from
Apr 23, 2014

Conversation

brendanzab
Copy link
Member

This pull request:

More information on the breaking changes can be found in the commit messages.

@brendanzab
Copy link
Member Author

r? @brson

@alexcrichton
Copy link
Member

If we're moving from &self to self, should Signed and Bitwise also be updated? (in a later PR)

It definitely does lock us in to thinking that the Float trait is only for the primitives, not for fancy wrapper structures (perhaps). (I'm a tad bit hesitant on this part)

@brendanzab
Copy link
Member Author

@alexcrichton It's either that or make everything by-ref for the float trait. It was just confusing because some were by-ref, others were by-val. We have to pick one.

And yes, I'm thinking Signed and Bitwise will be updated too.

cc. @thestinger

@brendanzab
Copy link
Member Author

Oh, and for basic math, 2f64.powf(&100.0) is rather ugly. :(

@brson
Copy link
Contributor

brson commented Apr 18, 2014

Whether to pick self or &self is a tough decision. The callers look much better here, in what is clearly the much more common case (than custom implementations of Float).

@brson
Copy link
Contributor

brson commented Apr 18, 2014

Needs rebase.

@brendanzab
Copy link
Member Author

Heading out now - I'll fix this when I get back. I'm ok to r=you @brson?

@thestinger
Copy link
Contributor

I do think it's a lot nicer by-value, but I'm worried that we're heading towards a situation where big integers and arbitrary precision floating point won't fit in to the language at all. It's not going to be possible to define a trait with a method called sqrt that's generic across all floating point types, since it's reserved. It would be nice to have a plan on how this kind of thing is going to be solved in a backwards compatible way.

@brendanzab
Copy link
Member Author

@thestinger: Yeah, that does keep me up at night too... :(

Move the rounding functions into the `std::num::Float` trait and then remove `std::num::Round`.

This continues the flattening of the numeric traits tracked in rust-lang#10387. The aim is to make `std::num` very simple and tied to the built in types, leaving the definition of more complex numeric towers to third-party libraries.

[breaking-change]
Make all of the methods in `std::num::Float` take `self` and their other parameters by value.

Some of the `Float` methods took their parameters by value, and others took them by reference. This standardises them to one convention. The `Float` trait is intended for the built in IEEE 754 numbers only so we don't have to worry about the trait serving types of larger sizes.

[breaking-change]
@brendanzab
Copy link
Member Author

@brson rebased

@brson
Copy link
Contributor

brson commented Apr 19, 2014

@thestinger These methods strike me as the built-in methods for the types - it would be better perhaps if they were just inherent methods. Maybe there can be another set of num traits for generic math.

@bachm
Copy link

bachm commented Apr 19, 2014

I much prefer the potentially generic function

hypot(a.x - b.x, a.y - b.y);

over this

(a.x - b.x).hypot(&(a.y - b.y));

@thestinger
Copy link
Contributor

A generic function needs to be written based on a trait method. Either way, a method is required and we've long since decided that we won't be duplication methods with wrapper functions. CTFE will allow methods to be imported/called as functions.

@brendanzab
Copy link
Member Author

@bachm Yeah, I agree that for functions where there is no obvious self parameter, the method call syntax looks weird compared with using a free-function (another example: x.min(y) vs min(x, y)). Apparently the uniform function call syntax changes will allow us to use and import methods like that in the future.

@brendanzab
Copy link
Member Author

@brson maybe we could have free, monomorphic functions in the {f32, f64} modules, then have an optional Float or Ieee754 trait (not exported in the prelude) to provide for a generic API? We could do the same for the integer APIs too.

@thestinger
Copy link
Contributor

I really don't want to go back to having redundant free functions. I don't think backwards compatibility concerns should override good API design when the issues can be solved in other ways.

@brendanzab
Copy link
Member Author

I was considering more the backwards compat question. The free functions would be the implementations, the trait would be a wrapper.

@thestinger
Copy link
Contributor

I would rather just figure out what we need to do to support arbitrary precision arithmetic with the same trait. For example, the return type could be given as a type parameter.

@brendanzab
Copy link
Member Author

Would arbitrary precision arithmetic support the full IEEE though? I would prefer the trait was as close as possible to the spec. (if there are holes, they could be added later)

@brendanzab
Copy link
Member Author

@thestinger could we have:

Float and IeeeFloat: Float?

@brson
Copy link
Contributor

brson commented Apr 22, 2014

This conversation is going in the direction of redesign. Is this current pull request an improvement we want to make?

@bjz we could also make the free functions fully generic, to reduce duplication, but by value, to make them callable like most expect, leaving the traits to use flexible by-ref calling. Then you just have the fully-generic traits that work with everything, and the free functions that incorrectly consume certain theoretical implementations.

@brendanzab
Copy link
Member Author

we could also make the free functions fully generic, to reduce duplication, but by value, to make them callable like most expect, leaving the traits to use flexible by-ref calling

@brson That's what we had in the past. My eventual goal was to have the free wrapper functions as the main way to call the float methods, and remove Float from the prelude. I think @thestinger and others weren't fans of the duplication though, and removed them in #13225.

@brson
Copy link
Contributor

brson commented Apr 23, 2014

Another idea: if by-val is bad for some types, then those types can implement Float for &T instead of T.

@brendanzab
Copy link
Member Author

I've thought of this before. You would have to return references, Like: impl<'a> Float for &'a T { fn foo(self) -> &'a T { ... } }

bors added a commit that referenced this pull request Apr 23, 2014
This pull request:

- Merges the `Round` trait into the `Float` trait, continuing issue #10387.
- Has floating point functions take their parameters by value.
- Cleans up the formatting and organisation in the definition and implementations of the `Float` trait.

More information on the breaking changes can be found in the commit messages.
@bors bors closed this Apr 23, 2014
@bors bors merged commit 2d9dfc6 into rust-lang:master Apr 23, 2014
@brendanzab brendanzab deleted the float-api branch April 30, 2014 20:43
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 9, 2023
…ue4u

fix: filter unnecessary completions after colon

close rust-lang#13597
related: rust-lang#10173

This PR also happens to fix two extra issues:

1. The test case in https://github.com/rust-lang/rust-analyzer/blob/master/crates/ide-completion/src/tests/attribute.rs#L778-L801 was never triggered in previous behavior.

after:

https://user-images.githubusercontent.com/26110087/201476995-56adf955-0fa7-4f75-ab32-28a8e6cb9504.mp4

<del>
2. completions were triggered even in invalid paths, like

```rust
fn main() {
    core:::::$0
}
```

```rust
#[:::::$0]
struct X;
```

</del>

only `:::` is excluded as discussed in rust-lang/rust-analyzer#13611 (comment)
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.

6 participants