Revise SVGPreserveAspectRatio constructor and callers so that the silly cast is not needed

RESOLVED FIXED in mozilla20

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: emk, Assigned: emk)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
I think some enum types would be even better than featureless uint_8.
(Assignee)

Comment 1

6 years ago
Created attachment 698231 [details] [diff] [review]
patch

I also renamed getters because they are infallible.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #698231 - Flags: review?(dzbarsky)
Comment on attachment 698231 [details] [diff] [review]
patch

I'm not an SVG peer.
Attachment #698231 - Flags: review?(dzbarsky) → review?(longsonr)
Comment on attachment 698231 [details] [diff] [review]
patch

Thanks for filing & posting a patch for this!  Patch mostly looks great -- I just have 2 concerns that I wanted to bring up.

>+class SVGPreserveAspectRatio MOZ_FINAL
> {
[...]
>-  uint8_t mAlign;
>-  uint8_t mMeetOrSlice;
>+  SVGAlign mAlign;
>+  SVGMeetOrSlice mMeetOrSlice;
>   bool mDefer;
> };

(1) While I'm in favor of the migration to enum values, I think we should double-check that it doesn't bring a penalty from failing to pack adjacent member-variables.  I can imagine that one of our compilers might refuse to pack mAlign and mMeetOrSlice into the same word, after your patch, because they're different types (even though they're still both backed by uint8_t).  So it might be worth doing two TryServer runs (one with your patch, one without), with a debugging printf inserted somewhere to print the size of the classes in question, to be sure there's no silly compiler-imposed penalty on the size of this struct.

>-  if (val.GetDefer()) {
>+  if (val.Defer()) {

(2) I don't think I'm in favor of this "remove 'Get' prefix" part.  My impression (which could be wrong) is that we're fine with "GetFoo()"-style getters, and we (usually) only drop the "Get" prefix if the method returns a pointer (which could be null). And in this case, I think that removing the prefix actually makes the code less clear -- the existing "GetDefer()" is clearly a getter, whereas "Defer()" sounds like a function that defers something.  (To a casual reader, "if (val.Defer())" sounds like we're deferring val (whatever that means), and proceeding if that succeeds)
(Assignee)

Comment 4

6 years ago
(In reply to Daniel Holbert [:dholbert] from comment #3)
> (1) While I'm in favor of the migration to enum values, I think we should
> double-check that it doesn't bring a penalty from failing to pack adjacent
> member-variables.  I can imagine that one of our compilers might refuse to
> pack mAlign and mMeetOrSlice into the same word, after your patch, because
> they're different types (even though they're still both backed by uint8_t). 
> So it might be worth doing two TryServer runs (one with your patch, one
> without), with a debugging printf inserted somewhere to print the size of
> the classes in question, to be sure there's no silly compiler-imposed
> penalty on the size of this struct.

static assert would be much faster to catch this :)
https://tbpl.mozilla.org/?tree=Try&rev=37997e806c38

> (2) I don't think I'm in favor of this "remove 'Get' prefix" part.  My
> impression (which could be wrong) is that we're fine with "GetFoo()"-style
> getters, and we (usually) only drop the "Get" prefix if the method returns a
> pointer (which could be null). And in this case, I think that removing the
> prefix actually makes the code less clear -- the existing "GetDefer()" is
> clearly a getter, whereas "Defer()" sounds like a function that defers
> something.  (To a casual reader, "if (val.Defer())" sounds like we're
> deferring val (whatever that means), and proceeding if that succeeds)

I'll revert renaming once the try-run passed.
(Assignee)

Comment 5

6 years ago
B2g compiler wasn't so smart, now testing with uint8_t version to verify things getting worse.
https://tbpl.mozilla.org/?tree=Try&rev=98a2b85e6aeb
(Assignee)

Comment 6

6 years ago
Created attachment 698288 [details] [diff] [review]
patch v2
Attachment #698231 - Attachment is obsolete: true
Attachment #698231 - Flags: review?(longsonr)
Attachment #698288 - Flags: review?(longsonr)
Attachment #698288 - Flags: review?(longsonr) → review+
Version: unspecified → Trunk
https://hg.mozilla.org/mozilla-central/rev/0b4ce0db7ecb
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
What's with the static assert? For example, on PowerPC bool is word-size, so it won't build as written.
I believe the static assert was there to be sure we didn't regress the size of SVGPreserveAspectRatio. (because this bug's patch switched it from using two adjacent uint8_t's to two adjacent uint8_t-backed enums, which could conceivably be packed less efficiently depending on how the compiler handles enums.)

Given that it didn't fail, I think we can safely remove it, if it's causing problems on other platforms.
(In reply to Daniel Holbert [:dholbert] from comment #10)
> Given that it didn't fail

...on tier 1 platforms, of course. :)  (It sounds like it *is* failing on powerpc, but I'd expect it always would've failed, since it's the bool that's taking up all the space there, and this bug didn't change the bool.)
Fair enough. I can file a separate bug or put a patch here, whatever is preferred.
Sounds great!  I'd prefer that it happen in a separate bug, since it's been months since this bug's original patch landed.

CC me; I'll grant swift r+. :)
Depends on: 847804
Done (bug 847804). Thanks!
You need to log in before you can comment on or make changes to this bug.