Here are some things I found a little confusing. I'm listing them all because I don't know enough about this class to write a better comment yet. :-\ > /* > * The Heap<T> class is a C/C++ heap-stored reference to a JS GC thing. It'd be great to make it clear that the JS GC heap counts. JSFunction contains one. What about the JS stack? > * > * Heap<T> wraps the complex mechanisms required to ensure GC safety for the > * contained reference into a C++ class that behaves similarly to a normal > * pointer. The grammatical structure here had me stumped for a second until I figured out that "into" goes with "wraps", not "reference" or "safety"; and that "a C++ class..." refers to Heap<T> itself, and not some arbitrary C++ class. More importantly -- "ensure GC safety" is an awfully vague, strong-sounding claim and had me thinking that Heap<T> was a root; it's not. > * Requirements for type T: > * - Must be one of: Value, jsid, JSObject*, JSString*, JSScript* Don't forget JS::Heap<nsXBLMaybeCompiled<nsXBLTextWithLineNumber> >! (slightly panic-stricken look) > */ The comment left me wondering how this edge actually gets marked. Might as well point out that's the enclosing object's responsibility. Also wondering what JS::Heap<T> has to do with js::HeapPtr<T>/HeapValue/HeapId, but that could be commented in js/src/gc instead of here... I feel a weird sense of deja vu... not sure why. Since I'm the jerk whining about this comment, which is better documentation than we have for 99.44% of all JSAPI features, I will be happy to patch it myself if you can spare a few minutes on IRC to explain to me in caveman language what I'm looking at...

Patch to update the JS::Heap doc a little. I also added very rudimentary comments in Barrier.h, just to point out that js::HeapValue != JS::Heap<Value>. These could do with their own documentation, and in fact I think there's already a bug for that.

(In reply to Jason Orendorff [:jorendorff] from comment #0) > Don't forget JS::Heap<nsXBLMaybeCompiled<nsXBLTextWithLineNumber> >! > (slightly panic-stricken look) That was honestly the simplest way to make that code work. But I don't want broadcast things like this by putting them the documentation :)

The must-be requirement should be statically asserted to prevent evildoing, if you're actually serious about it. (And I would do whatever it took to get rid of that XBL madness, to do so.)

Comment on attachment 824131 [details] [diff] [review] update-heap-doc Review of attachment 824131 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/RootingAPI.h @@ +189,1 @@ > * - Must be one of: Value, jsid, JSObject*, JSString*, JSScript* "should be one of:" => "must be one of" sounds a bit weird.

Comment on attachment 824131 [details] [diff] [review] update-heap-doc Review of attachment 824131 [details] [diff] [review]: ----------------------------------------------------------------- The new content looks good, but we should go a bit further with the changes. I think most of these corrections are to things I wrote in the first place, It's hard (for me at least) to piece together all the sentence fragments in this form into a final product, so lets give this one more quick look after the changes are made to make sure I didn't request something that sounds totally ridiculous. ::: js/public/RootingAPI.h @@ +168,5 @@ > > /* > + * The Heap<T> class is a heap-stored reference to a JS GC thing, for use > + * outside the JS engine. All members of heap classes that refer to GC things > + * should use Heap<T> (or possibly TenuredHeap<T>, described below). I think ", for use outside the JS engine" is sufficiently indicated by the location of the declaration. I think we should remove this and keep the first sentence extremely straightforward. @@ +173,4 @@ > * > + * Heap<T> is an abstraction that hides some of the complexity required to > + * ensure GC safety for the contained reference. It does this by providing a > + * C++ class that behaves similarly to a normal pointer. As Jason mentioned, "ensure GC safety" is probably too strong here. How about "maintain GC invariants". It sounds a bit odd that "Heap<T> ... provides a C++ class....". Lets just remove the second sentence of this paragraph (we'll add it back in below in a slightly altered form) and move the first sentence down to lead the next paragraph. @@ +177,4 @@ > * > + * To be more specific, it uses operator overloading to notify the GC every time > + * the value it contains is updated. This is necessary for generational GC, > + * which keeps track of all pointers into the nursery. Then we can leave off the joining "To be more specific," and add in the information from above as (additions underlined): "It uses operator overloading ... is updated _to provide a normal pointer interface_. Also, one space between sentences. @@ +181,3 @@ > * > + * Heap<T> objects must still be traced by their containing object in the usual > + * way. They do not make their contents a GC root. Lets assume less pre-existing knowledge here. How about: "Heap<T> instances must be traced when their containing object is traced to keep the pointed-to GC thing alive." And one space between sentences. @@ +182,5 @@ > + * Heap<T> objects must still be traced by their containing object in the usual > + * way. They do not make their contents a GC root. > + * > + * Heap<T> objects should only be used on the heap. GC references stored on the > + * C/C++ stack must use Rooted/Handle/MutableHandle instead. Lets upgrade this warning from "should" to "must". Sure, there are instances where it is okay to store these on the stack, but usually it is just going to be a sec-crit vulnerability. Plus, one space. Or rather, minus one space. @@ +189,1 @@ > * - Must be one of: Value, jsid, JSObject*, JSString*, JSScript* I guess this should be s/- Must be one of: /- /

Thanks for the comments, here's an updated patch.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3) > The must-be requirement should be statically asserted to prevent evildoing, > if you're actually serious about it. The class requires a specialization of js::GCMethods for type T, so it won't compile if someone tries to use it with a random type. That doesn't stop people providing their own specialization but it does prevent casual misuse. > (And I would do whatever it took to get rid of that XBL madness, to do so.) The situation there is that you have a union of JSObject* for a compiled function and a pointer to the uncompiled source. It was suggested that we should just separate these out into separate members, but that was rejected in review (see bug 877762 comment 38 and 39). An alternative would to use manual barriers, but this is bad because someone could break the code without realising if they adding a path that modified the pointer without triggering the barriers. I agree the current situation seems over-complicated though. Any suggestions would be welcome.

Comment on attachment 824545 [details] [diff] [review] update-heap-doc-v2 Review of attachment 824545 [details] [diff] [review]: ----------------------------------------------------------------- Nice! r=me ::: js/public/RootingAPI.h @@ +173,2 @@ > * > + * Heap<T> is an abstraction that hides some of the complexity required to main s/main/maintain/ @@ +182,2 @@ > * > + * Heap<T> objects should only be used on the heap. GC references stored on the One space.