Last Comment Bug 695097 - Split up frontend/Parser.{h,cpp}
: Split up frontend/Parser.{h,cpp}
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86 Mac OS X
: -- normal (vote)
: mozilla10
Assigned To: Jason Orendorff [:jorendorff]
:
Mentors:
Depends on: 695094
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-17 12:15 PDT by Jason Orendorff [:jorendorff]
Modified: 2011-10-18 05:39 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (1.11 MB, patch)
2011-10-17 12:20 PDT, Jason Orendorff [:jorendorff]
luke: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2011-10-17 12:15:55 PDT
This breaks out three smallish parts of frontend/Parser.{h,cpp}:

  - BytecodeCompiler.{h,cpp}    (the facade, js::Compiler)
  - ParseNode.{h,cpp}           (AST stuff: JSParseNode, JSDefinition, etc.)
  - FoldConstants.{h,cpp}       (js_FoldConstants)

Later, I'll rename the classes and functions into alignment with the filenames.
It's BytecodeCompiler.h rather than Compiler.h because we already have
js/src/methodjit/Compiler.h.
Comment 1 Jason Orendorff [:jorendorff] 2011-10-17 12:20:02 PDT
Created attachment 567529 [details] [diff] [review]
v1

I used "hg cp Parser.cpp ..." so that history is preserved and the patch consists mostly of - lines with very few + lines. Unfortunately that makes the patch super fragile. I made the patch based on some other patches in progress, so (I just realized) it almost certainly won't apply against tip. :-P  If you'd like one that does, that's ok -- I'll be producing one today anyway. But I think this is reviewable as-is.
Comment 2 Luke Wagner [:luke] 2011-10-17 12:37:32 PDT
Comment on attachment 567529 [details] [diff] [review]
v1

I agree with your high-level goal and using cp to preserve history.  My one request is that you update the #includes to match the style: https://wiki.mozilla.org/JavaScript:SpiderMonkey:C%2B%2B_Coding_Style#Includes.
Comment 3 Jason Orendorff [:jorendorff] 2011-10-17 15:00:29 PDT
Fixed and pushed.

http://hg.mozilla.org/integration/mozilla-inbound/rev/31b5cad155fb
Comment 4 Jason Orendorff [:jorendorff] 2011-10-17 15:51:58 PDT
Requires this follow-up:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e3f7294169d

Otherwise you get "Symbol undefined" linker errors. The -inl.h file contains the definition of the relevant template, so you need it in order to instantiate the method.

It worked fine in my local DEBUG builds because there is DEBUG-only code in other files that instantiates the same method. I'm not sure why it failed to link in the "debug" builds on the tinderboxen, but I noticed that we are passing both --enable-debug and --enable-optimize to configure.

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