763367 - Add support for [EnforceRange] and [Clamp]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I haven't actually tested these ... but I'm pretty sure they're right.

I also should probably write some parser tests, particularly for mutual exclusion of [EnforceRange] and [Clamp]

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Attaching the diff would help.

Btw I can get jlebar to review the parser changes if you like, but these are pretty straightforward and you're easily the third most familiar person.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 631791 [details] [diff] [review]
Patch

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

::: dom/bindings/PrimitiveConversions.h
@@ +134,5 @@
> +    MOZ_STATIC_ASSERT(std::numeric_limits<T>::is_integer,
> +                      "This can only be applied to integers!");
> +
> +    if (!MOZ_DOUBLE_IS_FINITE(d)) {
> +      return false;

Is this an uncatchable exception? I do think I'd like to see some tests... (Preferable in the W3C test suite, if there aren't any yet.)

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to :Ms2ger from comment #4)
> Comment on attachment 631791 [details] [diff] [review]
> Patch
> 
> Review of attachment 631791 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bindings/PrimitiveConversions.h
> @@ +134,5 @@
> > +    MOZ_STATIC_ASSERT(std::numeric_limits<T>::is_integer,
> > +                      "This can only be applied to integers!");
> > +
> > +    if (!MOZ_DOUBLE_IS_FINITE(d)) {
> > +      return false;
> 
> Is this an uncatchable exception? I do think I'd like to see some tests...
> (Preferable in the W3C test suite, if there aren't any yet.)

Ah, yes, they are :-/  I thought that the caller was doing the throwing, but it's not.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Lemme just steal this for now

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 646201 [details] [diff] [review]
Patch v2

The globalgen_targets bit doesn't make sense to me.

Why do you need value() functions instead of just static const char* class members?

The template args to PrimitiveConversionTraits_int_ToNumberHelper could use better names.  Especially U.  Perhaps call U "ValidityEnforcer" and s/constrict/enforceValidValue/?

The "v" argument of "constrict" seems to be unused.

Also PrimitiveConversionTraits_int_ToNumberHelper may be better off if it were called PrimitiveConversionTraits_ToCheckedIntHelper.

The [EnforceRange] behavior for 64-bit ints is incorrect.  It should be testing against 2^53-1, not the numeric limits for 64-bit int types.

You don't need the "int" bit in PrimitiveConversionTraits_int_Clamp and PrimitiveConversionTraits_int_EnforceRange (modulo your issues with 64-bit types above).

Why do you need to test MOZ_DOUBLE_IS_NEGATIVE_ZERO(d) in the clamp() case?  I don't see why that's needed.

The banker's rounding implementation is wrong for floats that are near a half-integer, just like other places in our tree (because you can have d + 0.5 == 1 even if d < 0.5, for example).  Not sure how much we care yet.  Might be good to just have a single correct impl of this algorithm that vaious places call...

I don't see why you need the manual specializations for PrimitiveConversionTraits for all the different types.  Won't this:

  template<typename T>
  struct PrimitiveConversionTraits<T, eEnforceRange> :
    PrimitiveConversionTraits_int_EnforceRange<T> {
  };

work?  Likewise for eClamp.

Can you make DisallowedConversion::converter private?  Seems like that would make sure attempts to use it just don't compile, as opposed to failing at runtime.

You need to pass an array of locations, not just a location, to WebIDLError.  We need better tests there.  :(

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Everything except for this:

(In reply to Boris Zbarsky (:bz) from comment #8)
> Why do you need value() functions instead of just static const char* class
> members?

is khuey's fault. I'll try to address the comments if he doesn't do it first :)

I used functions because I first tried

template<>
struct TypeName<int16_t> {
  static const char* value = "short";
};

but apparently you can only do in-class definition like this with numbers; so I went with the lazy option of functions.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> but apparently you can only do in-class definition like this with numbers;

Ah, then functions are fine.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v3

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Interdiff -w

This is an interdiff for PrimitiveConversions.h; rest of the changes are:
Codegen.py:
  Added to the getJSToNativeConversionTemplate docstring
  Removed # XXXbz need to add support for [EnforceRange] and [Clamp]
Makefile.in:
  Dropped globalgen_targets change
WebIDL.py:
  self.location --> [self.location]

and fixing bitrot

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 650615 [details] [diff] [review]
Patch v3

>+++ b/dom/bindings/PrimitiveConversions.h
>+  double toTruncate = (d < 0) ? d - 0.5 : d + 0.5;
>+  T truncated(toTruncate);
>+  if (truncated == toTruncate) {
>+    // Move towards 0.
>+    truncated = (d < 0) ? truncated + truncated % 2 :
>+                          truncated - truncated % 2;
>+  }

I should have caught this last time... This is wrong because % is a bit of a footgun in C/C++ (as in many programming languages).  In particular, it has the sign of the dividend.  So -1 % 2 == -1.

So say d == -0.5.  Then toTruncate == truncated == -1.  d < 0, truncated % 2 == -1, so we set truncated to -2.

Which is to say that the body of the if above should just be:

  // Move towards 0.  Note that (truncated % 2) has the same sign as |truncated| does,
  // so we always want to subtract here.
  truncated = truncated - truncated % 2;

r=me with that.  But please do file a followup on having a correct implementation of banker's rounding and using it here and in the other places we need it?

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/7682c01baef3

Landed with irc-r=bz for the double -> const double& changes, to placate MSVC.