Closed Bug 763367 Opened 8 years ago Closed 7 years ago

Add support for [EnforceRange] and [Clamp]

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: khuey, Assigned: Ms2ger)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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]
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #631791 - Flags: review?(bzbarsky)
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.)
(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.
Attachment #631791 - Attachment is obsolete: true
Attachment #631791 - Flags: review?(bzbarsky)
Lemme just steal this for now
Assignee: khuey → Ms2ger
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #646201 - Flags: review?(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.  :(
Attachment #646201 - Flags: review?(bzbarsky) → review-
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.
> but apparently you can only do in-class definition like this with numbers;

Ah, then functions are fine.
Attached patch Patch v3Splinter Review
Attachment #646201 - Attachment is obsolete: true
Attachment #650615 - Flags: review?(bzbarsky)
Attached patch Interdiff -wSplinter Review
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 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?
Attachment #650615 - Flags: review?(bzbarsky) → review+
Depends on: 781632
https://hg.mozilla.org/mozilla-central/rev/7682c01baef3

Landed with irc-r=bz for the double -> const double& changes, to placate MSVC.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.