Add rem_floor and rem_ceil

[clarfonthey]

Since rem_ceil is awkward for unsigned integers, I've put it under a separate feature flag, although the result is still in the same tracking issue.

I also removed the rhs > 0 check on the unsigned div_ceil since it's not necessary, and can be a bit confusing to readers. (The only alternative would be rhs = 0, which would panic before reaching this point.)

cc #88581

[rustbot]

r? @scottmcm

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[jhpratt]

Setting aside my lack of understanding on the use case, the implementation looks good to me.

[leonardo-m]

Setting aside my lack of understanding on the use case,

What's a use case of this?

[jhpratt]

I don't know…that's the point. But let's keep discussion of that in #88581.

[clarfonthey]

(FWIW I don't understand the docs error: I did define isize::rem_ceil so I don't know why it fails to link.)

[scottmcm]

r? joshtriplett
As a team member and the same person assigned to #94455

[clarfonthey]

(I decided to replace the link with a manual one so that it would pass the tests. No idea why it's failing otherwise.)

[joshtriplett]

The code looks fine here, assuming we're prepared to accept these from an API point of view. Given the questions expressed about use case, could you please file an ACP on these methods, specifically? (And since it came up in int_roundings, please do include use case information for unsigned and signed separately, since it's possible that one may be more supported than the other.)

[scottmcm]

Pondering: would who wants a consistent div and rem result would pair div_floor with rem_ceil?

That would look odd to me when reviewing, but it's also correct if rem_ceil means ceil(a % b), I think?

Thus maybe it would be nice to have merged versions of these, so you get both the quotient and the remainder in one call, with a consistent rounding direction for both.

[clarfonthey]

I'd be more than happy to write up an ACP for this, although it's worth clarifying that I did include use cases in the original tracking issue: #88581 (comment)

Ceiling remainder of unsigned integers is the one that I would consider weird and difficult to use, since it's asymmetrical: the remainder of ceiling division is always negative or zero, meaning that you have to return the absolute value of the result instead, or fail on non-exact division.

It's incredibly important and worth repeating: integer division and remainder always come in pairs. Yes, there are a lot of cases where you just throw out the remainder because you're wanting to round something, but I feel like the terms "division" and "remainder" gives a lot of clarity into the sorts of operations that would use these for: you're dividing something into a series of pieces, and even if you often toss out the "remainder," there's still the opportunity to make use of it. While we often abstract division as part of other operations (like averaging), it's good to remember what it actually physically represents.

[Dylan-DPC]

@clarfonthey i think you would still need an ACP for this

[clarfonthey]

From previous experience with ACPs, these are only for new features, and adding onto existing features is mostly what tracking issues are for. I would be happy to write up an ACP for rem_ceil since, as previously mentioned, it only makes sense for signed integers, but I think that rem_floor is reasonable to add for both.

[NCGThompson]

Signed % always panics on overflow (MIN / -1) regardless of the overflow checks compile option. At least this particular method will panic iff the % inside it panics. Therefor, the documentation is incorrect. The already merged ceil and floor div functions should be double checked with this in mind.

Once that is fixed I suggest explaining exactly what constitutes an overflow. (The remainder function never truly overflows, but it panics whenever the corresponding division would also panic.)

[NCGThompson]

The already merged ceil and floor div functions should be double checked with this in mind.

Submitted a PR for that and some other stuff. #119726

[joshtriplett]

r? libs-api

[dtolnay]

Thanks for the PR!

I read most of the comments in #88581.

I am on board with signed rem_floor and signed rem_ceil, but not unsigned rem_floor and unsigned unsigned_rem_ceil. Would you like to remove those from this PR and I can merge it, or make one more attempt at synthesizing the rationale in favor of having the entire set?

[clarfonthey]

Is there a particular reason you're against unsigned rem_floor? I know that it's equivalent to the default, but we already have unsigned div_floor, which is also the default, so, I'm not sure why remainder specifically should be excluded.

In terms of unsigned rem_ceil-- yes, I agree that this one is shaky, which is why I put it under a separate flag. I still think that it could be useful, and I'll put an explanation below, but if you'd rather not even do it then, I can remove it.

As I've said (and I sound like a broken record because I do genuinely believe this), a majority of cases that use division have a parallel case where the remainder could also be useful. For example, if you use div_ceil to round up a percentage bar, you could use rem_ceil when the rounding error makes the bar 100% to alternatively show the number of items remaining. Or, instead of deciding how many tasks are "left over" when dividing tasks among workers using flooring division, you could use ceiling division to get the number of no-op tasks to add to the list so it divides evenly.

Again, I agree that a lot of these fall into "but you could technically do it another way," but the primary goal of offering flooring and ceiling division was to make the intent of algorithms clearer, and I think offering remainders to all the division operators fits that, even if some are just identical to truncating division, and even if rem_ceil has to be negated in the unsigned case.

[dtolnay]

it's equivalent to the default, but we already have unsigned div_floor, which is also the default

I am going to be proposing for unsigned div_floor to be removed.

[clarfonthey]

I see. I'm not sure I like that approach since it does go against some of the other philosophies from the standard library-- for example, date algorithms are mentioned as a common case for div_floor, and switching from a signed timestamp to an unsigned timestamp would mean you have to change all the div calls, and vice/versa. I get if the standard library preferred not having "trivial" methods like these but it does have a lot of them.

[bors]

☔ The latest upstream changes (presumably #127133) made this pull request unmergeable. Please resolve the merge conflicts.

[rustbot]

Some changes occurred in src/tools/cargo

cc @ehuss

[clarfonthey]

That error is… very weird. I get it locally too, but not when I switch to the master branch, despite… nothing related being changed? I'm hoping it's a weird error that gets fixed and I can rebase on a newer commit that fixes it.

[ehuss]

It looks like you updated all the submodules. You should amend your commit and remove those.

[clarfonthey]

Huh, I had done a git diff but for some reason they didn't show up when I checked. I've fixed that.