Closed
Bug 1270009
Opened 8 years ago
Closed 8 years ago
Make CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES macro type-safe
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: xidorn, Assigned: xidorn)
Details
Attachments
(1 file, 1 obsolete file)
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50333/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50333/
Attachment #8748510 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bugzilla
Assignee | ||
Comment 2•8 years ago
|
||
It's probably not a common case which is worth the helper class... we probably should just remove the hack in CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES and make its callers always pass in a variable or constant.
Assignee | ||
Updated•8 years ago
|
Attachment #8748510 -
Attachment is obsolete: true
Attachment #8748510 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50693/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50693/
Attachment #8749022 -
Flags: review?(cam)
Assignee | ||
Comment 4•8 years ago
|
||
There are two alternatives: (1) create a helper class which can store the extra data with the iterator; (2) remove the extra variable and ensure all callsites pass in a constant or variable rather than an expression containing function call; (3) rewrite the macro to a function accepts a callback. As I mentioned in comment 2, (1) is probably not worth, since this is not a common case at all. For (2), it is impossible to ensure that new users would follow the convention and only pass in cheap expressions. I've tried (3) but in existing callsites, but it seems there is at least one callsite wants to break from the loop, and supporting so would lead to unnecessary code in other callsites.
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4) > There are two alternatives: I mean, three alternatives :)
Comment 6•8 years ago
|
||
What about, instead of adding a static_assert, we make the es_ declaration: es_ = (nsCSSProperty) ((enabledstate_) | CSSEnabledState(0)) Since we only have operator|() defined that takes two CSSEnabledState values, this shouldn't compile if enabledstate_ is any other type. Then we can avoid making the macro be multiple statements long.
Assignee | ||
Comment 7•8 years ago
|
||
Yeah, that probably makes sense to avoid making it multiple statement, although the compiler message would be a bit obscure.
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8749022 [details] MozReview Request: Bug 1270009 - Ensure CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES is type-safe. r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50693/diff/1-2/
Comment 9•8 years ago
|
||
Comment on attachment 8749022 [details] MozReview Request: Bug 1270009 - Ensure CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES is type-safe. r?heycam https://reviewboard.mozilla.org/r/50693/#review48613 ::: layout/style/nsCSSProps.h:20 (Diff revision 2) > #include "nsCSSProperty.h" > #include "nsStyleStructFwd.h" > #include "nsCSSKeywords.h" > #include "mozilla/CSSEnabledState.h" > #include "mozilla/UseCounter.h" > +#include "mozilla/TypeTraits.h" This can be removed.
Attachment #8749022 -
Flags: review?(cam) → review+
Assignee | ||
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/79d9809fed882064df4bf4978b289cc1c51ebda0 Bug 1270009 - Ensure CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES is type-safe. r=heycam
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/79d9809fed88
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•