Last Comment Bug 393499 - Clean up character lookup in nsCSSScanner
: Clean up character lookup in nsCSSScanner
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-08-23 21:42 PDT by Eli Friedman
Modified: 2008-06-11 17:44 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (12.44 KB, patch)
2007-08-23 21:42 PDT, Eli Friedman
bzbarsky: review+
dbaron: superreview+
dbaron: approval1.9+
Details | Diff | Splinter Review

Description Eli Friedman 2007-08-23 21:42:59 PDT
Created attachment 278002 [details] [diff] [review]
Patch
Comment 1 Eli Friedman 2007-08-23 21:52:42 PDT
Comment on attachment 278002 [details] [diff] [review]
Patch

This patch cleans up all the scattered checks for character types by adding a centralized set of inline helpers.  I think this makes the code a lot more readable compared to the scattered checks against 255/256 and various bit comparisons.  

Also a couple minor bug fixes to correctly classify \v as non-whitespace and make dimensions with a unit starting with a hyphen work correctly.  I'll make sure to update my parser tests to test those fixes.
Comment 2 Eli Friedman 2007-08-24 10:01:04 PDT
You can ignore the change to the "// From this point on, 0 <= ch < 256." comment.  That was part of some changes I subsequently realized were incorrect.
Comment 3 Boris Zbarsky [:bz] (TPAC) 2007-08-24 17:02:50 PDT
Comment on attachment 278002 [details] [diff] [review]
Patch

>Index: nsCSSScanner.cpp

>-  // From this point on, 0 <= ch < 256.

The point of that comment was that the bounds checks need not be performed when accessing the lextable.  But since the new code always performs them, is the comment relevant at all now?

r=bzbarsky, but please have dbaron sr.  He might have had reasons for the local vars, etc.
Comment 4 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2007-08-24 17:53:09 PDT
Comment on attachment 278002 [details] [diff] [review]
Patch

sr+a1.9=dbaron.  I'd say remove the comment;   Compilers should be able to optimize global variable access properly, so I don't think the local variables are needed.  I'm a little more worried about them optimizing the bitfield stuff (and the bounds checks), but there's only one occurrence of that (the bitfield combination, anyway).
Comment 5 Eli Friedman 2007-08-25 19:20:38 PDT
Checked in.

Note You need to log in before you can comment on or make changes to this bug.