876127 - nsStyleContext::CalcStyleDifference should assert if DO_STRUCT_DIFFERENCE is not called for all style structs

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

If a new style struct is added, nsStyleContext::CalcStyleDifference should be updated to call DO_STRUCT_DIFFERENCE for it.  We should assert in here if we neglect to add a DO_STRUCT_DIFFERENCE call when we do this.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: patch

Something like this.  There might be a clever way to do this as a static assertion though?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 754113 [details] [diff] [review]
patch

>+    styleStructCount++;                                                       \

Doesn't this need to be prefix ++ rather than postfix in order to call operator++(int) in opt builds?  Though I really think the operator++ on DebugOnly<T> is too tricky, and we ought to instead have an IF_DEBUG macro much like http://hg.mozilla.org/mozilla-central/file/a81bece350a1/toolkit/crashreporter/google-breakpad/src/third_party/glog/src/glog/log_severity.h#l67

r=dbaron if you make it compile for debug+opt in some reasonable way, though it would be nice to have a more global IF_DEBUG macro rather than the sneaky DebugOnly<T>::operator++(int).

Comment 3

[Cameron McCormack (:heycam)]

I don't know how much trickier operator++ on DebugOnly<T> is than operator=.  The class only defines a void-returning postfix ++ anyway, in both DEBUG and !DEBUG builds, so I think it should compile in opt builds.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, right, operator++(int) is postfix and operator++() is prefix.

I prefer prefix for general operations, though, given that it's the conceptually simpler operation (i.e., in an overloaded implementation, it's the one that doesn't involve making a copy).  It seems odd that the class allows postfix but not prefix.

I guess it's ok as is, then.

Comment 5

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd4593e3eac6

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bd4593e3eac6