Last Comment Bug 750907 - Move the marking declarations out of jsgcmark.h and into a header with minimal dependencies
: Move the marking declarations out of jsgcmark.h and into a header with minima...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla15
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 751003
  Show dependency treegraph
 
Reported: 2012-05-01 15:02 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-05-04 10:51 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (7.23 KB, patch)
2012-05-01 15:02 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
v2: Move jsgcmark into gc/, forward-declare some dependencies, and clamp down on over-inclusion (18.72 KB, patch)
2012-05-01 16:23 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
wmccloskey: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-01 15:02:09 PDT
Created attachment 620078 [details] [diff] [review]
Patch

As part of the new object/property/elements representation, I'm introducing a class that encapsulates the current Shape* stored in ObjectImpl.  (The idea is that we won't be conflating the ideas of a single property and an object's list of properties in the same representation.)  That class will contain a HeapPtr, which means I need to have the write barrier methods defined before the properties class is defined.  That means I need the corresponding marking methods declared by that point.  That means I need jsgcmarker.h.

I thought that depended on jscompartment.h, but it turns out it doesn't, actually -- that include can be removed at no cost.  I didn't realize this at the time, tho, so I wrote the patch.  I think it's a better state than previously -- moves stuff out of js/src into more precise directories, minimizes dependencies a bit, and generally makes things cleaner.  So how about this?

I tested this by removing all the contents of jsalloc.cpp and putting a single #include "gc/Markers.h" in it, then making sure that file compiled.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-01 16:23:00 PDT
Created attachment 620124 [details] [diff] [review]
v2: Move jsgcmark into gc/, forward-declare some dependencies, and clamp down on over-inclusion
Comment 2 Matt Brubeck (:mbrubeck) 2012-05-03 14:58:03 PDT
Sorry, backed out on inbound because of build failure:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26738df8a4e0
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-03 16:46:30 PDT
No worries, relanded and green after enough time that it's passed the original failure (it built all the way for me locally, too):

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd3e28b8adfb

The problem was jsatom.h needed to see the HeapPtrAtom typedef (you can't predeclare types that are typedefs, unfortunately) that I moved to vm/String.h -- a non-installed header.  The simple fix was to use HeapPtr<JSAtom> rather than HeapPtrAtom, permitting the non-kosher #include to be removed.
Comment 4 Ed Morley [:emorley] 2012-05-04 10:51:27 PDT
(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #3)
> The problem was 

http://30.media.tumblr.com/tumblr_m11ykqEl0P1rrf1eeo1_500.jpg

:-)

https://hg.mozilla.org/mozilla-central/rev/bd3e28b8adfb

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