Closed Bug 1022381 Opened 7 years ago Closed 7 years ago
To functions in Unit Transforms .h allow templating with rects, creating footgun
While working on bug 1018387, I accidentally changed a call from TransformTo<LayoutDevicePixel>(...) to: TransformTo<LayerRect>(...) Note the use of LayerRect rather than LayerPixel. This compiled and ran just fine, but had very peculiar runtime behaviour which seemed mostly correct but wasn't actually. I ended up wasting a lot of time before I tracked this down. It would be nice if we could prevent this at compile-time.
The root of the problem here is that templates like PointTyped or RectTyped can be instantiated with LayerRect (which is RectTyped<LayerPixel>) as an argument, instead of LayerPixel, without the compiler complaining. I propose the following solution: - Introduce a type trait IsPixel<T>, which would evaluate to false by default, but would be specialized to return true for CSSPixel, LayerPixel etc. - In PointTyped, RectTyped, etc., static_assert that the template argument satisfies IsPixel. (With Concepts Lite (proposed for post-C++14), this would look much nicer: we would make a Pixel concept, have CSSPixel, LayerPixel etc. model it, and then declare the "type" of PointTyped's template parameter to be Pixel, which would cause the compiler to enforce that it is only instantiated with a type modeling the Pixel concept.)
That sounds like a reasonable solution.
This patch implements the proposed resolution. Please feel free to flag for additional review if you feel it's appropriate.
Assignee: nobody → botond
Attachment #8437774 - Flags: review?(bugmail.mozilla)
Whoops, had some typos in there. Fixed.
Comment on attachment 8437776 [details] [diff] [review] bug1022381.patch Review of attachment 8437776 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the FrameMetrics and Units changes. gfx/2d/ should be reviewed by Bas.
Build-only Try push: https://tbpl.mozilla.org/?tree=Try&rev=b9bd7ac1eb7b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Attachment #8437776 - Flags: review?(bas) → review+
Thanks Bas! (I didn't realize I landed this without waiting for your review - sorry!)
You need to log in before you can comment on or make changes to this bug.