Last Comment Bug 757690 - Merge TokenStream::TokenStream() and TokenStream::init()
: Merge TokenStream::TokenStream() and TokenStream::init()
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Nicholas Nethercote [:njn]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: UntangleFrontEnd
  Show dependency treegraph
 
Reported: 2012-05-22 18:25 PDT by Nicholas Nethercote [:njn]
Modified: 2012-05-24 09:29 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (18.31 KB, patch)
2012-05-22 18:25 PDT, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2012-05-22 18:25:41 PDT
Created attachment 626277 [details] [diff] [review]
patch

TokenStream has an init() that cannot fail and therefore isn't necessary.
This patch:

- Merges TokenStream::init() with TokenStream::TokenStream().
  A knock-on effect is that Parser::Parser gets five more arguments, and
  Parser::init() gets five fewer.

- Un-defaults five (yes, five!) arguments in Parser::Parser().  That many
  default arguments gives me with willies;  explicit is better than
  implicit, etc, etc.
  
  I suspect some of the places that use |foldConstants = true| don't need it
  -- that's what happens when you overdo the default args -- but I'm being
  conservative and not changing them for now.

- Inits Parser::tempPoolMark in the constructor just to be safe.

- Moves the setting of tokenStream.setStrictMode() based on
  JSOPTION_STRICT_MODE from Parser::init() to TokenStream::TokenStream(), a
  much better place for it.

- Merges TokenStream::TokenBuf::{init,TokenBuf}.  The split is no longer
  needed.

- Removes a bunch of out-of-date comments from above
  TokenStream::TokenStream's declaration -- TokenStream::close no longer
  exists, and TokenStream doesn't use JSContext::tempLifoAlloc, AFAICT.

- Adds a missing failure check after the parser.init() call in
  shell/js.cpp.
Comment 1 Jason Orendorff [:jorendorff] 2012-05-23 14:52:48 PDT
Comment on attachment 626277 [details] [diff] [review]
patch

Nice cleanup.

In Parse in shell/js.cpp:
>+                      "<string>", /* lineno = */ 0, cx->findVersion(),

While you're in here, please change the lineno to 1.

(0 is always wrong for lineno. It's a very common mistake.)
Comment 2 Nicholas Nethercote [:njn] 2012-05-23 16:11:37 PDT
> >+                      "<string>", /* lineno = */ 0, cx->findVersion(),
> 
> While you're in here, please change the lineno to 1.
> 
> (0 is always wrong for lineno. It's a very common mistake.)

In conjunction with "<string>" as the filename, I thought this signified "the line number doesn't mean anything".  But I'm happy to change this one, and the similar one earlier in the file.
Comment 3 Nicholas Nethercote [:njn] 2012-05-23 17:46:47 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2105611f8968

I forgot the lineno=1 change, I did it in a follow-up:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c84b9365f6fa
Comment 4 Ed Morley [:emorley] 2012-05-24 09:26:53 PDT
https://hg.mozilla.org/mozilla-central/rev/c84b9365f6fa
Comment 5 Ed Morley [:emorley] 2012-05-24 09:29:28 PDT
Sorry, and https://hg.mozilla.org/mozilla-central/rev/2105611f8968

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