Closed Bug 561516 Opened 11 years ago Closed 11 years ago

[FIX]Switch pseudo-class matching from an array of function pointers to a switch

Categories

(Core :: CSS Parsing and Computation, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(1 file)

It benchmarked a tiny little bit slower on Mac back when, but we now have more arguments to the functions and maintaining this code is a pain if we change the arguments at all.  With the new arguments, I wouldn't be surprised if a switch is faster, since most cases don't actually need all the extra variables, and since on x86 we're now having to pass args to the callback functions on the stack.
Attached patch Like soSplinter Review
Attachment #441238 - Flags: review?(dbaron)
Comment on attachment 441238 [details] [diff] [review]
Like so

>-PR_STATIC_ASSERT(NS_ARRAY_LENGTH(sPseudoClassInfo) >
>+PR_STATIC_ASSERT(NS_ARRAY_LENGTH(sPseudoClassBits) >
>                    nsCSSPseudoClasses::ePseudoClass_NotPseudoClass);

Can't this be "== NotPseudoClass + 1" instead of "> NotPseudoClass"?


Don't you want to check sPseudoClassBits *outside* the switch and then 
make the default case of the switch be an NS_ABORT_IF_FALSE(PR_FALSE,
"...")?


I hope you were careful when you figured out what should be a return
PR_FALSE and what should be a break.  I just checked that there weren't
any |return PR_TRUE| left. :-)


r=dbaron with that
Attachment #441238 - Flags: review?(dbaron) → review+
> Can't this be "== NotPseudoClass + 1" instead of "> NotPseudoClass"?

Yes.

> Don't you want to check sPseudoClassBits *outside* the switch

I could, I suppose.  I really wonder whether it would compile to the same thing...  Probably better to have the bits check outside the switch to be safe.

> I hope you were careful when you figured out what should be a return
> PR_FALSE and what should be a break.

I tried, yes.  I'll double-check them all again before pushing, and run tryserver of course.
Well, I definitely had one bug at least: for :root I had an == where I should have had a !=.  Fixed locally, and review comments addressed.  Pushed to try again, and still need to do the double-checking.
And I did verify that at least Linux gcc creates a jump table here.
Pushed http://hg.mozilla.org/mozilla-central/rev/330848610d95
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.