865036 - Add a |To SafeCast<To>(From from)| method that asserts cast-correctness then returns the cast for numbers

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Comment 1

[Jeff Walden [:Waldo]]

Attachment: 1 - mozilla::IsFloatingPoint, mozilla::IsArithmetic

Comment 2

[Jeff Walden [:Waldo]]

Attachment: 2 - mozilla::Is{S,Uns}igned

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Add a Casting.h header to hold various casting methods, right now including a SafeCast method that asserts in-rangeness. NOT REVIEWED YET

The header only holds SafeCast now, but in the longer run it seems to make sense to add BitCast and CastToFunction/CastFromFunction (to encapsulate the __extension__ bit needed to disable function-to-data cast warnings) here as well.  Ergo, new header.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: More-working Casting.h

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Tests

Comment 6

[Nathan Froyd [:froydnj]]

For avoidance of doubt, the rationale for SafeCast is something like:

1) Programmer needs to make a potentially value-altering conversion;
2) -Wconversion warns about an implicit conversion;
3) Programmer adds static_cast, pretty sure that everything will just work out; but
4) We'd really like a check that we never run into the problematic cases ("trust, but verify").

Is that correct?

Comment 7

[Nathan Froyd [:froydnj]]

Sigh, bugzilla title collision.

Comment 8

[Nathan Froyd [:froydnj]]

Also, bonus points if you modify the JS engine to use IsFloatingPoint and IsUnsigned (I think it's IsUnsigned) in the appropriate places, would eliminate a few unused function warnings.

Comment 9

[Jeff Walden [:Waldo]]

(In reply to Nathan Froyd (:froydnj) from comment #6)
> 1) Programmer needs to make a potentially value-altering conversion;
> 2) -Wconversion warns about an implicit conversion;
> 3) Programmer adds static_cast, pretty sure that everything will just work
> out; but
> 4) We'd really like a check that we never run into the problematic cases
> ("trust, but verify").
> 
> Is that correct?

Exactly this, excepting that -Wconversion isn't enabled -- now -- so this doesn't do anything one way or the other about the warning situation.  But if we enable that warning in js/src, as the dependent bug plans, this would get used a bit for that.

Unfortunately IsFloatingPoint and IsUnsigned can't be used in the places you're thinking of, because of Uint8ClampedArray.  (One type that flows in there is uint8_clamped, so uint8_clamped(-1) and uint8_clamped(0) won't be compile-time constants, so you'll hit the caveat about using those templates with user-defined types.)

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 741087 [details] [diff] [review]
More-working Casting.h

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

Works for me.  Would be interested to hear your thoughts on reusing bits from CheckedInt.h

::: mfbt/Casting.h
@@ +21,5 @@
> +enum FromSignedness { FromIsSigned, FromIsUnsigned };
> +
> +template<typename From,
> +         typename To,
> +         FromSignedness = IsSigned<From>::value ? FromIsSigned : FromIsUnsigned,

I assume this is an unnamed template parameter?  Learn something new every day.

@@ +23,5 @@
> +template<typename From,
> +         typename To,
> +         FromSignedness = IsSigned<From>::value ? FromIsSigned : FromIsUnsigned,
> +         ToSignedness = IsSigned<To>::value ? ToIsSigned : ToIsUnsigned>
> +struct BoundsCheckActual;

Nit: CheckedInt.h uses *Impl for classes like this; bits of TypeTraits.h use *Helper.  I don't have a preference between those two, but I think either one is better than *Actual.

@@ +73,5 @@
> +  public:
> +    static bool check(const From from) {
> +      typedef typename Conditional<sizeof(To) >= sizeof(From), To, From>::Type
> +              LargerType;
> +      const To MinValue = -2 * (To(1) << (CHAR_BIT * sizeof(To) - 2));

std::numeric_limits would be fantastic here and elsewhere in this header.

Failing that, I think it'd be better to reuse the MinValue and MaxValue templates from CheckedInt.h here, probably by separating them into their own header.  Or do we want to minimize the dependence of CheckedInt.h on MFBT headers?

If we don't want to reuse bits from CheckedInt.h, I think computing things consistently between the FromIsUnsigned/ToIsSigned and FromIsSigned/ToIsSigned cases would be best.

Comment 11

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a8a16157576
https://hg.mozilla.org/integration/mozilla-inbound/rev/441c2a4b1dde
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5adb2a42135

(In reply to Nathan Froyd (:froydnj) from comment #10)
> Nit: CheckedInt.h uses *Impl for classes like this; bits of TypeTraits.h use
> *Helper.  I don't have a preference between those two, but I think either
> one is better than *Actual.

Impl's fine with me.

> std::numeric_limits would be fantastic here and elsewhere in this header.

It would be nice, indeed.  Unfortunately there's that matter of their not necessarily working for <stdint.h> types, because they're not necessarily the same as some built-in type in C++.

> Failing that, I think it'd be better to reuse the MinValue and MaxValue
> templates from CheckedInt.h here, probably by separating them into their own
> header.  Or do we want to minimize the dependence of CheckedInt.h on MFBT
> headers?

"want to" is perhaps slightly strong.  :-)  But I think we're pretty much doing it, given that header's being copied/used elsewhere, even if it'd be nicer code-wise to not have to.  Oh well, not too much burden generally.

> If we don't want to reuse bits from CheckedInt.h, I think computing things
> consistently between the FromIsUnsigned/ToIsSigned and
> FromIsSigned/ToIsSigned cases would be best.

Makes sense, done.

Now that I've resurrected my blog (it got hacked/spammed, and I didn't have time to deal so .htaccess'd it off for awhile), this is close to the top of the list of things to blog about.  Not quite at the top, I have one other commitment that needs to happen first, and one other mfbt thing to blog about as well, but definitely up there.

Comment 12

[(PTO May 7-12) Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8a8a16157576
https://hg.mozilla.org/mozilla-central/rev/441c2a4b1dde
https://hg.mozilla.org/mozilla-central/rev/f5adb2a42135