rational: Specify invariants, no more negative denominators

[ExpHP]

Two things in here:

---

First is a small commit which closes #208. It clarifies the rounding behavior of some methods and
makes formatting more uniform.

---

The second lays down the law on negative denominators, so to speak,
as (a) not all methods accounted for them properly to begin with, and
(b) those that did can now be simplified. In particular:

• #[derive(PartialEq,Eq)] instead of delegating to Ord.
• Methods of Signed mostly just delegate to the numerator now.

No outwardly visible behavior should notably change as a result of
these updates, except for people who were using new_raw for
nefarious purposes. (but don't take my word for it; please do check!)

This also adds a specification that the denominator of zero is 1.
(really, this is part of the same rule by which numer and denom
must share no factors in common; stated generally, the actual
rule is that "numer and denom must be coprime")

---

Note: I was going to stick a debug_assert_eq!(&ret, &ret.reduced())
in new_raw, but didn't after realizing I would have to add a
T: fmt::Debug bound. :/

[ExpHP]

The scope of this PR has changed a bit: I added a small commit to fix recip(), and a slightly bigger one that aims to explicitly forbid negative denominators (which makes several methods simpler).

[cuviper]

I think these enforced invariants are good, but let's get a second opinion. @hauleth?

Note: I was going to stick a debug_assert_eq!(&ret, &ret.reduced()) in new_raw, but didn't after realizing I would have to add a T: fmt::Debug bound. :/

You could use debug_assert!(ret == ret.reduced()).

[ExpHP]

You could use debug_assert!(ret == ret.reduced()).

Thanks for pointing out this possibility.

I did this along with a couple of other iterative improvements on the docs in another branch. I was too timid to add those commits to this branch as, to be honest, I don't collaborate too often and so I'm not sure what a reasonable-sized diff for a PR is!

(also I feel they slightly change the theme of the PR; whereas the edits in the current PR are mostly just trying to simplify the implementation, the debug_assert commit quite strongly reflects my attitude that new_raw is public API (and not just a wrongly-exposed implementation detail). This is a sentiment that not everyone may agree with)

[cuviper]

the debug_assert commit quite strongly reflects my attitude that new_raw is public API (and not just a wrongly-exposed implementation detail).

Well, yes, whether it was an accident or not, it's public now. So the question is how much we should be willing to change it. Now that I think more, adding even a debug assertion might be overkill, breaking a use that would have worked before, even if suboptimal.

I think it should probably go ahead and enforce the (~new) invariant that the denominator is always positive. Not by asserting, unless its zero, but just go ahead and fix up negative denominators. This should be an easy check, especially compared to the gcd reduction that new_raw users are presumably trying to avoid.

How does that sound?

[ExpHP]

Now that I think more, adding even a debug assertion might be overkill, breaking a use that would have worked before, even if suboptimal.

I'm not sure I understand what you mean here. When you say "a use" here are you thinking about something specific? (or suggesting the possibility of there being something we haven't thought about?)

This should be an easy check, especially compared to the gcd reduction that new_raw users are presumably trying to avoid.

This is true, though on the other hand I feel there is also a big difference between a small cost and zero cost. I have to wonder whether the addition of a branch here could serve as a barrier to other optimizations. But I am probably overthinking things.

On the other hand yet, since it is evident from the commit history that people accepted the existence of negative denominators (most likely due to the recip bug), I do have to wonder if there might be code out there that does truly depend on e.g. new_raw(3, -1).is_positive() == false. If such code exists, then your solution is necessary to avoid breakage...

[cuviper]

My point is that since this is a public API, we don't know how it is used. Changing anything that was allowed before is possibly a breaking change, unless you can justify it as a bug fix. So I think recip allowing a zero denominator is definitely a bug, but you can't say the same for new_raw changes. The "contract" is in its documentation, which states that it "Creates a ratio without checking for denom == 0 or reducing." Even the check I wanted violates this! :(

Now perhaps you see why I was sad that this is public in #208 (comment). I think we'll have to hold the invariant changes until we're ready for breaking changes. (Someday we'll get around to a 0.2 en masse.)

[ExpHP]

I see. I will scale it back to just the documentation improvements and recip.

[cuviper]

Thanks. I know it's a pain being so conservative, but I try to be very careful because num is one of the most-downloaded crates (currently #11).

[cuviper]

Heh -- I didn't mean to link to an issue there, but #11 happens to be about rational! You're addressing recip; switch to is_zero() in Ratio::new and we might even close that one. The point about overflows has been addressed in Rust itself, RFC 560, but there might be some cases where we could do better to avoid unnecessary overflows.

[ExpHP]

Closing now in favor of #213