Let's keep the comment style consistent in the function by switching to //.

I followed Gecko and copied the comments as is, I will switch them to // for consistency.

The markdown link from my suggestion is now broken as the text inside [...] has to match some from a link in the preceding paragraph. You can fix it with:

By the way if you use an IDE like Visual Studio Code or Zed, if you hover over the function name it should give you a formatted view of the rustdoc and let you test clicking on links.

Hrm. Shouldn't you be truncating truncate here instead of to_truncate? to_truncate has been rounded, but not truncated via the cast.

With that change does Servo still pass the tests?

If that doesn't work I also wonder about the as f64 here. Theoretically this function should only be called if D is an integral type. Gecko ensures this with a static cast, but that doesn't seem to be possible in Rust unfortunately. If the above doesn't work, does the following?

No — to_truncate is intentional here and matches the Gecko implementation,the tie check must compare against the value after the ±0.5 adjustment and truncation step, using the already cast value would lose the information needed to detect exact .5 ties.

I ran the Servo tests before committing, and they are passing with this change.

Here is the Gecko code:

 T truncated = static_cast<T>(toTruncate);

  if (truncated == toTruncate) {
    /*
     * It was a tie (since moving away from 0 by 0.5 gave us the exact integer
     * we want). Since we rounded away from 0, we either already have an even
     * number or we have an odd number but the number we want is one closer to
     * 0. So just unconditionally masking out the ones bit should do the trick
     * to get us the value we want.
     */
    truncated &= ~1;
  }

The value that is modified here is truncated which has already been cast to the final type. I believe that Gecko code is trying to compare the rounded value to the rounded+cast value and then modify the rounded+cast value if they are equal. In your case you are comparing the rounded value to the rounded+cast value and then modifying the rounded (but not cast value). Instead you cast to i64, which I think is redoing the rounding cast but to a type of a potentially different size than D.

FWIW, I think this might work fine when doing this operation from f64 to i64, but I suspect it will not work exactly the same was as Gecko in the general case. That could potentially be why this still passes tests.

I think I see your concern, In Gecko the low bit is cleared on the already cast value, while in the Rust version the even adjustment is computed via an i64 intermediate and then cast back to D, right?

given the WebIDL bounds, this produces the same result today, but I agree the difference can be confusing since the comments are copied from Gecko. I can adjust the code to operate directly on truncated if that’s clearer.

Yes, I think we should just use the truncated value here and then maybe put a warning in the rustdoc saying that the function should only be used when the target is an integer type. Thank you!

So this is:

1. Converting an integer to a float (I think this is what truncated.cast() does)
2. Converting the float back to an integer. (as i64)
3. Doing a bitwise operation on the integer. (& !1)
4. Converting the integer back to a float. (as f64)
5. Converting that float back to an integer. (.cast())

In the Gecko code it directly does the bitwise operation on the integer. 😦 I wish we could do the same here. Two options:

1. Do the truncation and bit flipping in i64 and then convert at the end. We cannot even do this easily as the various integer types (such as u8) do not implement As<i64> This could be done with TryInto but I don't know the performance implications or how performance sensitive this code is.
2. Ensure that D implements the necessary bit operations as well as num-traits crate's One trait. This might work, but I have not tried it.

Maybe someone with a better grasp of Rust generics can help here.

In the meantime I've dismissed my review so it doesn't block this change.

Yes truncated = truncated & !D::one(); works here.