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

Reform rational #6471

Merged
merged 5 commits into from
May 14, 2013
Merged

Reform rational #6471

merged 5 commits into from
May 14, 2013

Conversation

gifnksm
Copy link
Contributor

@gifnksm gifnksm commented May 14, 2013

std::ratio module contains BigRational type, but the type is not usable by following reasons.

  • Ratio::new requires T: Copy + Num + Ord, but BigInt is not implicitly copyable, because it contains unique vector.
  • BigInt is not implements Num

So, I rewrite Ratio as follows.

  • Ratio requires T: Clone + Integer + Ord.
    • Copy -> Clone: to be able to use BigRational
    • Num -> Integer: It is incorrect that a rational number constructed by two non-integer numbers.
  • BigInt implements Num and Orderable which are required by Integer bound

@huonw
Copy link
Member

huonw commented May 14, 2013

My reasoning for Num rather than Integer was a Ratio of two polynomials (for example) is sensible, although it might make sense for polynomials to implement Integer.

This allows creating `Ratio<T>` which `T` is non-implicitly copyable types
such as `BigInt`.
@gifnksm
Copy link
Contributor Author

gifnksm commented May 14, 2013

@huonw I think it makes sense that Rational of polynomials too.
I rebase to revert 41eaa97. (It may not break other commits)

@huonw
Copy link
Member

huonw commented May 14, 2013

Cool! (Nice clean-up.)

@huonw
Copy link
Member

huonw commented May 14, 2013

Actually thinking about it a bit more, it would be better to use the gcd implementation on the Integer trait than to have the one in the rational.rs. (Since the performance of the type-specific one will presumably be much better.)

So I think that the Integer bound is better than the Num bound.

@gifnksm
Copy link
Contributor Author

gifnksm commented May 14, 2013

@huonw It is correct in the performance sense, but it is strange in the naming convention sense (polynomials is Integer? I think not.)
I think it better that gcd method belongs to the other trait, such as CommonDivisor or Factor, if gcd supports polynomials.
(This may be the issue that should be discussed in #4819)

For now, in this pull request, using Integer instead of Num, and I stop working on reverting 41eaa97.
Please review.

@huonw
Copy link
Member

huonw commented May 14, 2013

I agree about the naming, but names are always hard, and I added a note to #4819.

(I don't have magical r+ powers, by the way.)

@gifnksm
Copy link
Contributor Author

gifnksm commented May 14, 2013

Oh, I didn't know you don't have the power, sorry.
I like the phrase "magical r+ powers" :)

bors added a commit that referenced this pull request May 14, 2013
`std::ratio` module contains `BigRational` type, but the type is not usable by following reasons.
* `Ratio::new` requires `T: Copy + Num + Ord`, but `BigInt` is not implicitly copyable, because it contains unique vector.
* `BigInt` is not implements `Num`

So, I rewrite `Ratio` as follows.
* `Ratio` requires `T: Clone + Integer + Ord`.
  * `Copy` -> `Clone`: to be able to use `BigRational`
  * `Num` -> `Integer`: It is incorrect that a rational number constructed by two non-integer numbers.
* `BigInt` implements `Num` and `Orderable` which are required by `Integer` bound
@bors bors closed this May 14, 2013
@bors bors merged commit da9c1fb into rust-lang:incoming May 14, 2013
@gifnksm gifnksm deleted the reform-rational branch May 16, 2013 13:45
flip1995 pushed a commit to flip1995/rust that referenced this pull request Dec 20, 2020
Fix blessing of new reference files

Adding of new reference files wasn't handled correctly. It was trying to
read a file that didn't exist yet.

Instead of unwrapping, we now treat a missing reference file as empty
(`Vec::new`). This makes the following conditional work. We then also
have to re-read the reference file after it was being copied. This
second read is technically the same as in the old shell script, but
wasn't really obvious there. The shell script did a `-s` test which
reads the file as well.

changelog: internal: Fix `cargo dev bless` when new reference files are added
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.

3 participants