1276066 - Add Union with "components" member to math classes to enable array access to members

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[:kip (Kearwood Gilbert)]

Adding an array member to access the components of vector, size, and matrix classes reduces the code needed when passing all of the members to functions.

This also aims to discourage dangerous patterns such as incrementing the address of members to access the other members.

Bug 1250244 (Implement WebVR 1.0 API) uses this for construction of Float32Array's.

Comment 1

[:kip (Kearwood Gilbert)]

Attachment: MozReview Request: Bug 1276066 - Add Union with "components" member to math classes to enable array access to members.

- Adding an array member to access the components of vector, size,
  and matrix classes reduces the code needed when passing all of the
  members to functions.

Review commit: https://reviewboard.mozilla.org/r/55566/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55566/

Comment 2

[Botond Ballo [:botond]]

Comment on attachment 8757056 [details]
MozReview Request: Bug 1276066 - Add Union with "components" member to math classes to enable array access to members.

https://reviewboard.mozilla.org/r/55566/#review52520

This looks good to me, but as a foundational change to core Moz2D classes, I think Bas should review as well.

Comment 3

[Nathan Froyd [:froydnj]]

Just FYI: this pattern is technically not permitted by the C++ spec, though it's an extremely common compiler extension to enable it to work.

Comment 4

[:kip (Kearwood Gilbert)]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> Just FYI: this pattern is technically not permitted by the C++ spec, though
> it's an extremely common compiler extension to enable it to work.
Indeed, it appears to be standard in C11 but not C++11.  Does this affect any platforms we are interested in?

I chose this approach since it doesn't compromise performance and we don't need to update the call sites to the members; however, an alternative would be to implement accessor functions.

Would it help to add some static assertions to verify the memory alignment and packing?

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to :kip (Kearwood Gilbert) from comment #4)
> (In reply to Nathan Froyd [:froydnj] from comment #3)
> > Just FYI: this pattern is technically not permitted by the C++ spec, though
> > it's an extremely common compiler extension to enable it to work.
> Indeed, it appears to be standard in C11 but not C++11.  Does this affect
> any platforms we are interested in?

This is standard in C11?  I hate reading the C spec, but the closest thing I could find in the C spec was 6.7.2.1p16, which says:

"The size of a union is sufficient to contain the largest of its members. The value of at
most one of the members can be stored in a union object at any time. A pointer to a
union object, suitably converted, points to each of its members (or if a member is a bit-
field, then to the unit in which it resides), and vice versa."

The second sentence appears to rule out the "store one member, retrieve another member" approach.  But again, I hate reading the C spec, so it's entirely possible there's other language that I missed!

I don't think any of our platforms actually do bad things to this pattern (I know we rely on it in a couple of other places...).

> I chose this approach since it doesn't compromise performance and we don't
> need to update the call sites to the members; however, an alternative would
> be to implement accessor functions.
> 
> Would it help to add some static assertions to verify the memory alignment
> and packing?

I think static assertions would be an excellent idea, regardless of the avenue we decide to take.  There's a proposed type trait for determining whether a structure contains no internal padding, but a bunch of static assertions would serve just as well here.

Comment 6

[Bas Schouten (:bas.schouten)]

Comment on attachment 8757056 [details]
MozReview Request: Bug 1276066 - Add Union with "components" member to math classes to enable array access to members.

https://reviewboard.mozilla.org/r/55566/#review52722

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b31e8fca349

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b31e8fca349