(Spun off from bug 617005.) The attached patch is a revert of changes to nativegen.py (and to the generated files) that cause it to emit GCMember, not DRCWB, smart pointers. However this is incompatible with some code in the Player still. Steven explains: "The problem is that in some contexts, GCMember<> requires a full declaration of the class, not merely a forward declaration, and the Flash headers haven't been properly scrubbed to allow for this." As a consequence of the revert, Tamarin code still contains uses of DRCWB, but those are the only uses.

I'll look into this today; the problematic bits are 100% in Flash and hopefully require only minor header tweaking.

After doing some initial investigation, I suspect this will require nontrivial work to make this work with existing precompiled-header usage in AIR, due to circular dependencies. For instance: the precompiled header wants to include avmglue.h and avmglue-classes.hh. Since avmglue-classes.hh includes a class that uses GCMember<ContentPlayerObject> [which is defined in HousekeepingGlue, not avmglue], we must include ContentPlayerGlue.h before avmglue-classes.hh... but ContentPlayerGlue.h, in turn, uses GCMember'ed classes from HousekeepingGlue.h and HousekeepingGlue-classes.hh... but those can't go before avmglue-classes.hh because they include classes that inherit from EventDispatcherObject. Oy. I'm hunching that the way to solve this is to emit each classes glue code in a separate file, with its dependencies included properly -- which is doable and likely to be useful in the long run anyway, but a few days work to implement the changes and iron out all the Flash/AIR project changes.

...all that said: I'll bet a dollar that we will still end up with circular dependencies that we can't easily resolve, meaning that we'll need to have a way to force GCMember<> to work with forward-declared classes.

This should be a matter of separating inline functions that use the "not fully defined GCMember" into a separate file that does fully define that GCMember type. Is that not what you're seeing?

Hmm, could be, will investigate

Yeah, that's pretty much it. Separating the inline functions into a -inlines.h file may make this problem more tractable. It's still gonna be awkward, though: we can't include GCMember-inlines.h via avmplus.h (or GC.h, etc), as it will have to be manually included *after* all necessary class definitions. Doable but yuck. We really need general header-file-rethinking sanity in the FR codebase

OK, this approach will probably work (and not have circularity), but would be a massive code-churn: we have to add #include "GCMember-inlines.h" (and also GCMemberBase-inlines.h) *after* all possible class definitions. As a practical matter, given the widely varying PCH (or lack thereof) in various FR platforms, this means adding that include to several hundred CPP files...

This patch doesn't solve the problem, but is a potentially necessary precondition: moves all the inlined methods into separate -inlines.h files. Still included from MMgc.h, though, so it doesn't help the circularity problem, but arguably good hygiene in its own right.

Brent has pointed out a possible confusion on my part: it's likely the inlined getter/setters that are the problem, not the member declarations themselves. This might be considerably simpler to finesse with a some further tweaking of nativegen, investigating...

Comment on attachment 524252 [details] [diff] [review] Move all inline methods in GCMember/GCMemberBase into -inlines.h files SR+ on principle, though I await further information about the "best" approach. Note Ruchi has been changing these files too, I just landed a patch for her (copy constructor fixes). Be extra careful when merging.

(In reply to comment #10) > Comment on attachment 524252 [details] [diff] [review] > Move all inline methods in GCMember/GCMemberBase into -inlines.h files > > SR+ on principle, though I await further information about the "best" approach. After some more fiddling, I think I may be able to make this work without heroic measures, just a lot of Flash-specific munging. (That said, I think moving the GCMember inlines into separate files is still a good idea.)

Comment on attachment 524252 [details] [diff] [review] Move all inline methods in GCMember/GCMemberBase into -inlines.h files switching R? to Ruchi since she's touching these files -- Ruchi, if you approve, please feel free to go ahead and land this for me, since I'm OOO for a couple of weeks.

Comment on attachment 524252 [details] [diff] [review] Move all inline methods in GCMember/GCMemberBase into -inlines.h files Is the patch with the latest TR? Probably not as I dont see my latest changes in GCMemberbase-inlines.h Looks good to me. Did we just fix the ctors in case of GCObject and GCRoot? Not all the ctors were explicit before.

(In reply to comment #13) > Is the patch with the latest TR? Probably not as I dont see my latest changes > in GCMemberbase-inlines.h latest as of yesterday afternoon. would you prefer to add your changes to this patch and land it in one go? > Looks good to me. Did we just fix the ctors in case of GCObject and GCRoot? Not > all the ctors were explicit before. I made all the ctors explicit, as IMHO it's best practice for these sort of classes.

(In reply to comment #14) > (In reply to comment #13) > > Is the patch with the latest TR? Probably not as I dont see my latest changes > > in GCMemberbase-inlines.h > > latest as of yesterday afternoon. would you prefer to add your changes to this > patch and land it in one go? They already landed. Thanks to Lars. My commit access is on its way.

OK, want me to update my patch with your changes and re-post for re-review?

I can if you want but I dont see a point in that.

Comment on attachment 524252 [details] [diff] [review] Move all inline methods in GCMember/GCMemberBase into -inlines.h files pushed to TR as 6174:f8e5c430eb79

This is finally ready to land in Flash, so attaching the final patch and rubber-stamping it for myself (since the original patch was R+'ed long ago)

TR 6291:8811a1c53b59

Comment on attachment 531103 [details] [diff] [review] Final patch R+ to remove SteJohn's request for a self-review of the patch and remove bug from review request reports.

(In reply to comment #20) > TR 6291:8811a1c53b59 This patch appears to have not landed in tr-serrano but is already in perforce. Reopening bug so that this gets resolved.

(In reply to comment #22) > (In reply to comment #20) > > TR 6291:8811a1c53b59 > > This patch appears to have not landed in tr-serrano but is already in > perforce. Reopening bug so that this gets resolved. Oh wait, this is targeted to FUTURE and I need to compare to a different branch.