On Tue, Jun 15, 2010 at 9:02 PM, Luke Zarko <[email protected]> wrote: > I'm not quite sure what you mean wrt the reinterpret_cast. Do we keep a > Factory* and a Heap* both inside Isolate but have them point to the same > runtime object?
Yes. > Is this done elsewhere in the code? Yes, in api. >Will it interfere with compiler optimizations? Maybe (we might get strict aliasing warnings). Yet given that this is done already, we're not creating a new problem. > Would it be better to rename the methods that can > cause GC rather than placing them into a different facade class? It may be better in general, but I think such refactorings are beyond the scope of the branch (unless they simplify something significantly for us, which is not the case here). Still the way it's done in this patch is very confusing. Having AllocateFoo and NewFoo in a single class, where the first one returns a raw pointer and the second one may cause GC and returns a handle, could be dangerous. With a more consistent naming scheme (e.g. having "Raw" prefix for raw functions: "RawNewFoo" and "NewFoo") it might work. Thanks, Vitaly > On Tue, Jun 15, 2010 at 9:47 AM, <[email protected]> wrote: >> >> Luke, >> >> There's a clear separation between Heap and Factory. The former doesn't >> handle >> allocation failures and operates on raw pointers, the latter can trigger >> GC and >> operates on handles. I think it's better to keep the separation. If you're >> worried about performance implications just reintepret_cast<Heap*>(this) >> in >> Factory methods. >> >> >> Thanks, >> Vitaly >> >> http://codereview.chromium.org/2834002/show > > -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
