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

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

(Blocks 1 bug)

Trunk
mozilla23
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

No description provided.
Attachment #741074 - Flags: review?(nfroyd)
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.
Attachment #741076 - Flags: review?(nfroyd)
Attachment #741076 - Attachment is obsolete: true
Attachment #741076 - Flags: review?(nfroyd)
Attachment #741087 - Flags: review?(nfroyd)
Posted patch TestsSplinter Review
Attachment #741088 - Flags: review?(nfroyd)
Summary: Add a |To SafeCast<To>(From from)| method that asserts cast-corrects then returns the cast → Add a |To SafeCast<To>(From from)| method that asserts cast-corrects then returns the cast for numbers
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?
Flags: needinfo?(jwalden+bmo)
Summary: Add a |To SafeCast<To>(From from)| method that asserts cast-corrects then returns the cast for numbers → Add a |To SafeCast<To>(From from)| method that asserts cast-corrects then returns the cast
Sigh, bugzilla title collision.
Summary: Add a |To SafeCast<To>(From from)| method that asserts cast-corrects then returns the cast → Add a |To SafeCast<To>(From from)| method that asserts cast-correctness then returns the cast for numbers
Attachment #741073 - Flags: review?(nfroyd) → review+
Attachment #741074 - Flags: review?(nfroyd) → review+
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.
(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.)
Flags: needinfo?(jwalden+bmo)
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.
Attachment #741087 - Flags: review?(nfroyd) → review+
Attachment #741088 - Flags: review?(nfroyd) → review+
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.
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.