Last Comment Bug 697179 - Miscellaneous parser/scanner cleanups
: Miscellaneous parser/scanner cleanups
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-25 10:45 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-11-09 05:22 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Extract ParseNode::append from ParseNode::newBinaryOrAppend, for slightly more readable code (4.48 KB, patch)
2011-10-25 10:45 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
cdleary: review+
Details | Diff | Splinter Review
Remove nearly all initializers from the TokenKind enum (11.62 KB, patch)
2011-10-25 10:46 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
cdleary: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-25 10:45:58 PDT
Created attachment 569421 [details] [diff] [review]
Extract ParseNode::append from ParseNode::newBinaryOrAppend, for slightly more readable code

These two patches are a total grab-bag of unrelated stuff, but I need a bug to dump them under as they're not quite worthy of drivebying.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-25 10:46:47 PDT
Created attachment 569422 [details] [diff] [review]
Remove nearly all initializers from the TokenKind enum
Comment 2 Nicholas Nethercote [:njn] 2011-10-27 22:44:54 PDT
Comment on attachment 569422 [details] [diff] [review]
Remove nearly all initializers from the TokenKind enum

Review of attachment 569422 [details] [diff] [review]:
-----------------------------------------------------------------

At various times in the past I've found the initializers in this table very helpful for working out which TokenKind a particular integer (e.g. printed out with debugging printfs for profiling purposes) represents.  Do we really need to remove them?
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-01 12:01:19 PDT
Initializing token kinds as happens now can make for large diffs when token kinds are changed, as I'm currently doing in an effort to simplify tokenizing and parsing code.  Having initializers for all of them makes it easy to make a mistake and duplicate something.  Such mistakes are pernicious and tricky to debug.

If what you want is a way to correlate token kinds with names, would a helper function returning a const char* naming it do the trick?  That could be easily printf'd and similar.
Comment 4 Chris Leary [:cdleary] (not checking bugmail) 2011-11-04 14:59:07 PDT
Comment on attachment 569422 [details] [diff] [review]
Remove nearly all initializers from the TokenKind enum

Review of attachment 569422 [details] [diff] [review]:
-----------------------------------------------------------------

I'm okay with it on maintainability grounds. We can add a TokenKindToString function under #ifdef DEBUG for debugging purposes if we want, or put one value per line?
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-08 10:36:31 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0afc4c9fd0c
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b6b6ee628f8 (with a debug TokenKindToString function, and also with one value per line for good measure)

The latter change cries out for a .tbl file, to be sure (although there are a couple things repeating like this does that not-repeating can't).  I filed bug 700719 for that.

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