1260851 - FloatingPoint<double>::kMaxSafeInteger

Status: NEW

(Core :: MFBT, defect)

Description

[Jason Orendorff [:jorendorff]]

As requested in bug 1255128 comment 5.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Part 1: Add mozilla::FloatingPoint<T>::kMaxSafeInteger

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Part 2: Use the new constant where appropriate in the JS engine

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 8736439 [details] [diff] [review]
Part 1: Add mozilla::FloatingPoint<T>::kMaxSafeInteger

Review of attachment 8736439 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/FloatingPoint.h
@@ +125,5 @@
>     */
>    static_assert(sizeof(T) == sizeof(Bits), "Bits must be same size as T");
> +
> +  static_assert(Base::kExponentShift + 1 < 64, "kMaxSafeInteger needs to have a bigger type");
> +  static const uint64_t kMaxSafeInteger = (uint64_t(1) << (Base::kExponentShift + 1)) - 1;

We could make this

    static MOZ_CONSTEXPR_VAR T kMaxSafeInteger =
        T((Bits(1) << (Base::kExponentShift + 1)) - 1);

if we're willing to sacrifice constant-ness when we lack constexpr support.  Seems somewhat doubtful we can/should, for all of these.

So let's also add

#ifndef MOZ_CONSTEXPR_VAR
#  error "kMaxSafeInteger should be made a T now that we have reliable constexpr support."
#endif

so this gets improved to that ideal state once that's possible.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8736441 [details] [diff] [review]
Part 2: Use the new constant where appropriate in the JS engine

Review of attachment 8736441 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for these, even if I made you "omg".  :-)

::: js/src/jsnum.h
@@ +273,1 @@
>      double len = JS::ToInteger(argument);

// Step 1.

@@ +273,3 @@
>      double len = JS::ToInteger(argument);
>      if (len <= 0)
>          return 0;

// Step 2.

@@ +278,1 @@
>      return len;

// Step 3.
return js::Min<double>(len, mozilla::FloatingPoint<double>::kMaxSafeInteger);

or if we had constexpr support, just std::min (but we don't).

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:sg, could you have a look please?
For more information, please visit auto_nag documentation.