FixedDecimal mutating functions should use active verbs

[sffc]

It occurred to me when reviewing #2898 that the function names we chose for rounding do not have an active verb: "half_even", etc. Should they be active verbs?

Some of the functions are fine, like "trunc". I feel like "floor" is understood to be a verb in this context, too.

[eggrobin]

Some of the functions are fine, like "trunc".

I must say trunced makes me think of trounced than truncated (but agreed, it is more understandable than the various flavours of rounding to nearest).

[sffc]

Discuss with:

• @younies
• @eggrobin
• @skius

[skius]

Some thoughts: I think there's nothing majorly wrong with the current way. If we wanted to change it, here's some suggestions:

• apply_ as prefix for the unit methods, and the current unit names for the chaining methods, e.g., apply_half_trunc(mut &self); and half_trunc(self).
• _chain (or _chained) suffix for the chaining methods, e.g., half_trunc(&mut self) and half_trunc_chain(self).

Another thought, however, is that I had some trouble understanding all the methods (e.g., half_even and half_expand) from their names and their docs. After realizing that we're in the context of rounding, half_even was clear after consulting the wikipedia article on Rounding. There's nothing in the article about "half expand" (or "expand", for that matter), however, and even checking the first few Google results for "half expand rounding" doesn't explain what it is. I assume it is the same as f64::round, i.e., "rounding away from zero".

Since we are discussing changing names, maybe adding a round somewhere in the name might help, and it could also turn the name into an active verb. But otherwise, because the crate is essentially only used internally, I don't think this is actionable enough to open an issue/change the docs (unless other people strongly agree).

Verdict: I don't think the issue of active verbs is a big deal, but changing names might clear things up a bit. I do agree with @eggrobin that trunc/truncate is a commonly understood name for that specific kind of rounding.

[sffc]

• @eggrobin - "expand" came out of nowhere.
• @sffc - (discusses history of how ECMA arrived at it)
• @Manishearth - We could provide both "away_from_zero" and "expand" as function names
• @younies - Following the standardization, even from ECMA, seems better than inventing a new name
• @sffc - We have ECMA in our charter so I think it is fine to follow ECMA. If we have long names, they should be for everything.
• @eggrobin - The ECMA decision was in a desire for short identifiers which may not be shared by this project. I would agree with long names for everything.
• @Manishearth - We do have some pressure toward small identifiers, but it is not strong
• @Manishearth - We rely on ECMA for scope and operations, and we take inspiration for API from there but also ICU, etc. It's going to take a while for "expand" to peculate through even the ECMA community. We should have API names that make sense for Rust people and people in other programming languages. "expand" is a choice that doesn't make sense with intuition.
• @sffc - Adding these APIs is 18 more functions.
• @sffc - "expand" is the only short name in the industry for this operation.
• @sffc - It's doing a positive service by evangelizing this word; it helps make it a consistent word and makes it in effect more horizontal.
• @eggrobin - IEEE 754 is very horizontal
• @Manishearth - We could go all in on the long names and deprecate all the short names... my priority order is to fix confusion in the API
• @skius - Java uses the word "up"
• @sffc - It's not clear that this is confusing. If the function's docs are not clear, we should just make them more clear. I don't think the new information we have from 1.0 is enough to justify backtracking.
• @sffc - IEEE 754 does not list all 9 rounding modes; only a subset
• @eggrobin - It is an extensible scheme
• @skius - Rust uses "round" for expand
• @Manishearth - It's confusing but makes sense if you're only exposing 3
• @sffc - I would not be opposed to an API named "round" aliasing to half_expand
• @Manishearth - Yeah maybe in Rust only
• @eggrobin - That seems like it adds to the pile of confusing APIs

Conclusion: Add better documentation for FixedDecimal. We are unable to reach consensus on a set of names for the functions.

[sffc]

A slate of names to consider in the future (suggested by @eggrobin):

• round_ceiling
• round_flooring
• round_expanding
• round_truncating
• round_half_ceiling
• ...
• rounded_ceiling
• ...

[sffc]

With the addition of @jedel1043's new RoundingIncrement functions, @markusicu noted that the matrix of these rounding functions is very large and unwieldy. I believe we now have 36 rounding functions, 4 for each of the 9 rounding modes.

We need separate *_to_increment because of code size. However, we could reduce the matrix by eliminating the self-moving function variants, since this can be fairly ergonomically modeled using Rust code blocks.

Also incorporating @eggrobin's naming scheme from the previous post, it would mean clients would need to change the following:

formatter.format(
    FixedDecimal::from(123usize)
        .multiplied_pow2(-2)
        .half_expanded(-1))

to

formatter.format({
    let mut fd = FixedDecimal::from(123usize);
    fd.multiply_pow2(-2);
    fd.round_half_expanded(-1);
    fd
})

Of course the assembly code between these two examples should be exactly the same.

[sffc]

Seeking consensus on the following proposals for 2.0:

Part 1

Remove the self-moving rounding functions in order to reduce API bloat. See previous post.

Seeking consensus from:

• @sffc
• @Manishearth
• @eggrobin
• @echeran
• @jedel1043

Part 2

Rename all of the rounding functions to have round_* in the name, and fully spell out their English names. The full list:

• round_ceiling(&mut self, position: i16)
• round_ceiling_to_increment(&mut self, position: i16, increment: RoundingIncrement)
• round_half_ceiling(&mut self, position: i16)
• round_half_ceiling_to_increment(&mut self, position: i16, increment: RoundingIncrement)
• round_flooring(&mut self, position: i16)
• round_flooring_to_increment(&mut self, position: i16, increment: RoundingIncrement)
• round_half_flooring(&mut self, position: i16)
• round_half_flooring_to_increment(&mut self, position: i16, increment: RoundingIncrement)
• round_truncating(&mut self, position: i16)
• round_truncating_to_increment(&mut self, position: i16, increment: RoundingIncrement)
• round_half_truncating(&mut self, position: i16)
• round_half_truncating_to_increment(&mut self, position: i16, increment: RoundingIncrement)
• round_expanding(&mut self, position: i16)
• round_expanding_to_increment(&mut self, position: i16, increment: RoundingIncrement)
• round_half_expanding(&mut self, position: i16)
• round_half_expanding_to_increment(&mut self, position: i16, increment: RoundingIncrement)
• round_half_even(&mut self, position: i16)
• round_half_even_to_increment(&mut self, position: i16, increment: RoundingIncrement)

Seeking consensus from:

• @sffc
• @Manishearth
• @eggrobin
• @echeran
• @jedel1043

Part 3

To increase discoverability, add aliases for the most common rounding functions:

• floor == round_flooring
• ceil == round_ceiling
• trunc == round_truncating
• expand == round_expanding
• round == round_half_even

Seeking consensus from:

• @sffc
• @Manishearth
• @eggrobin
• @echeran
• @jedel1043

Part 4 Alternative A

To further reduce API bloat but at the cost of choices when it comes to code size, we can make all round_* functions take a RoundingIncrement. This proposal is contingent on Part 3 being approved: those functions will not take a RoundingIncrement parameter and thereby get optimal code size.

• round_ceiling(&mut self, position: i16, increment: RoundingIncrement)
• round_half_ceiling(&mut self, position: i16, increment: RoundingIncrement)
• round_flooring(&mut self, position: i16, increment: RoundingIncrement)
• round_half_flooring(&mut self, position: i16, increment: RoundingIncrement)
• round_truncating(&mut self, position: i16, increment: RoundingIncrement)
• round_half_truncating(&mut self, position: i16, increment: RoundingIncrement)
• round_expanding(&mut self, position: i16, increment: RoundingIncrement)
• round_half_expanding(&mut self, position: i16, increment: RoundingIncrement)
• round_half_even(&mut self, position: i16, increment: RoundingIncrement)

Seeking consensus from:

• @sffc
• @Manishearth
• @eggrobin
• @echeran
• @jedel1043

Part 4 Alternative B

To reduce API bloat even further, we can merge all of the "unusual" rounding functions down to just one function that takes an enumeration argument.

Again, this is dependent on Part 3 so that we provide something for code-size-conscious clients.

There is some bikeshedding to do here. A strawperson:

pub enum RoundingMode {
    Ceil,
    Floor,
    Trunc,
    Expand,
    HalfCeil,
    HalfFloor,
    HalfTrunc,
    HalfExpand,
    HalfEven,
}

impl FixedDecimal {
    pub fn round_to_mode(&mut self,
        position: i16,
        mode: RoundingMode) { ... }

    pub fn round_to_mode_and_increment(&mut self,
        position: i16,
        mode: RoundingMode,
        increment: RoundingIncrement) { ... }
}

Seeking consensus from:

• @sffc
• @Manishearth
• @eggrobin
• @echeran
• @jedel1043

[sffc]

As part of this issue, we should also address the documentation feedback @markusicu left in the RoundingIncrement API Review here: https://docs.google.com/document/d/1gKZCaiTG2eeyxrIKUF2tf6PIsaPCIBaUVRXD-JDh6dI/edit#heading=h.7pm5mstzwo40

[jedel1043]

Agreed on the general idea of parts 1-3. For part 4, I strongly tend towards B because it would considerably simplify the API usage for users who take the rounding mode programmatically. Example of the current usage:

https://github.com/boa-dev/boa/blob/7da0f270cd8582fb030a4816775f7944acd5c2b2/core/engine/src/builtins/intl/number_format/options.rs#L722-L739

match mode {
    RoundingMode::Ceil => number.ceil_to_increment(position, multiple),
    RoundingMode::Floor => number.floor_to_increment(position, multiple),
    RoundingMode::Expand => number.expand_to_increment(position, multiple),
    RoundingMode::Trunc => number.trunc_to_increment(position, multiple),
    RoundingMode::HalfCeil => number.half_ceil_to_increment(position, multiple),
    RoundingMode::HalfFloor => number.half_floor_to_increment(position, multiple),
    RoundingMode::HalfExpand => number.half_expand_to_increment(position, multiple),
    RoundingMode::HalfTrunc => number.half_trunc_to_increment(position, multiple),
    RoundingMode::HalfEven => number.half_even_to_increment(position, multiple),
}

With the proposed change, this would simplify to:

number.round_to_mode_and_increment(position, mode, multiple)

[markusicu]

I don't feel strongly about the round_ prefix, but the suffixes that follow bother me. "ceiling" is a noun, something to round towards. "truncating" is the infinitive of a verb. "flooring" sounds like a known (as in a flooring store), but I suspect that you intend it to be understood as another verb infinitive. And adding "-ing" would be comical on "half_even".

[markusicu]

On Alternative B, you could drop the _to_mode suffix/infix.

[sffc]

On Alternative B, you could drop the _to_mode suffix/infix.

My proposal is that round performs default half-even rounding, which means we need some other name for the function that takes the RoundingMode argument.

[jedel1043]

My proposal is that round performs default half-even rounding

This is consistent with Python, but I think it's more common to use half-expand rounding by default. (Rust uses that in its f32/f64::round method).

[sffc]

My proposal is that round performs default half-even rounding

This is consistent with Python, but I think it's more common to use half-expand rounding by default. (Rust uses that in its f32/f64::round method).

Okay, how about making the list of functions in Part 3 be

• (ceil, floor, trunc, expand)
• round_half_even
• round_half_expand

I would like to not add a function named round that takes a large matrix of arguments due to code size and performance. Better to have users who need the extra power "opt in" by using a longer function name.

[eggrobin]

Scripsit Markus:

the suffixes that follow bother me. "ceiling" is a noun, something to round towards. "truncating" is the infinitive of a verb. "flooring" sounds like a known (as in a flooring store), but I suspect that you intend it to be understood as another verb infinitive. And adding "-ing" would be comical on "half_even".

4B does have the advantage of sidestepping the whole discussion of how to avoid silliness of either round half evenings or round half floors (presumably these are found in half-apartments of spherical buildings?), while retaining the word round that gives the hapless reader a chance to figure out what this mysterious evening is about¹.

Footnotes

1. As partially captured in the minutes from last summer I still think it was a dreadful idea not to go with the decades-old round (to nearest ties )?(away from 0|towards (0|(positive|negative) infinity)|to (odd|even)) which is standard throughout the numerical analysis literature, but at least with the word round one knows that one needs to map to that. ↩

[Manishearth]

Can be done pre-2.0 because fixed_decimal is not icu4x-versioned

[sffc]

It is in FFI and therefore impacts 2.0

[sffc]

My understanding is that the proposed set of functions is:

• ceil(position)
• floor(position)
• trunc(position)
• expand(position)
• either round(position) or round_half_even(position)
• optional round_half_expand(position)
• round_with_mode(RoundingMode, position)
• round_with_mode_and_increment(RoundingMode, position, RoundingIncrement)

Does that match everyone's expectations?

[sffc]

Also, do we still keep the -ed versions?

With fewer functions, maybe we can afford to keep them now.

[sffc]

Start: 11:05

• @sffc - I had suggested removing the self-moving functions earlier, but now that we will have many fewer rounding functions, maybe we can keep them because the matrix is not as big.
• @robertbastian - &mut self functions are annoying to work with, I'd prefer keeping them

Conclusion: keep self-moving functions.

@sffc @robertbastian