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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

Details

Attachments

(1 file, 1 obsolete file)

      No description provided.
Assignee: nobody → bugzilla
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.
Attachment #8748510 - Attachment is obsolete: true
Attachment #8748510 - Flags: review?(jwalden+bmo)
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.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4)
> There are two alternatives:
I mean, three alternatives :)
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.
Yeah, that probably makes sense to avoid making it multiple statement, although the compiler message would be a bit obscure.
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 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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/79d9809fed882064df4bf4978b289cc1c51ebda0
Bug 1270009 - Ensure CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES is type-safe. r=heycam
https://hg.mozilla.org/mozilla-central/rev/79d9809fed88
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.