Re: [PATCH] Reordering interface base initialization

[Tim Janik <timj gtk org>]

On Wed, 27 Aug 2003, Owen Taylor wrote:

> On Tue, 2003-08-26 at 20:25, Owen Taylor wrote:
>
> > There were spare bits in ClassData where this information could
> > be put but none in IFaceEntry. If you wanted to get funky, you
> > could probably squeeze the the necessary information into the
> > 2 spare alignment bits at the bottom of iface_entry->vtable, but
> > that would be icky. A better possibility would be to keep a
> > look-aside data structure holding information about what interfaces
> > have been initialized for each class we are currently initializing.
>
> I went ahead and added the lookaside.

what's the point in the lookaside structure?
is this all about sparing the initialization state info from
IFaceEntry? (the lookaside munching makes your patch buttugly
if i may say that ;)
i don#t quite like type_iface_vtable_init_Wm() being split up
into two functions btw, that duplicates a bunch of code and
makes the logic inside the two functions only slightly simpler.

> +struct _ClassInitInfo
> +{
> +};

i could be misstaken, but i think empty structures are a gcc
extension. (yep, i noticed you don't use it so it's porlly
a temporary left over)

>  static gboolean
> -type_iface_vtable_init_Wm (TypeNode *iface,
> -                          TypeNode *node)
> +type_iface_vtable_base_init_Wm (TypeNode *iface,
> +                               TypeNode *node)
>  {
>    IFaceEntry *entry = type_lookup_iface_entry_L (node, iface);
>    IFaceHolder *iholder;
>    GTypeInterface *vtable = NULL;
>    TypeNode *pnode;
> -
> +
>    /* type_iface_retrieve_holder_info_Wm() doesn't modify write lock for returning NULL */
>    iholder = type_iface_retrieve_holder_info_Wm (iface, NODE_TYPE (node), TRUE);
>    if (!iholder)
> -    return FALSE;      /* we don't modify write lock upon FALSE */
> +    return FALSE;

i don't like that comment being removed. i know you added a blurb
mentioning this before the function now, but i want to be reminded
about the lock behaviour when comming across this return ;)

> +  init_info.init_node = node;
> +  if (CLASSED_NODE_N_IFACES (node) > MAX_STACK_IFACE_STATES)
> +    init_info.iface_states = g_malloc (CLASSED_NODE_N_IFACES (node));
> +  else
> +    init_info.iface_states = stack_iface_states;

for what it's worth (since i don't like the lookaside stuff anyways),
you could have made your code a good deal simpler just for forgoing
the stack allocation here. class/iface initialization isn't as time
critical or frequently called, that a single malloc would make a
difference (and if it were, the g_slist_prepend() you use would
be potentially way more offending).

> Also wrote a fairly comprehensive
> test case that adds classes at various different points in the
> during class init.

great, been waiting for that.
can you commit/intergrate the test case already (yes it
obviously breaks for everyone, but that doesn't matter).

> Still need to improve the test case a bit more:
>
>  - Needs to test adding interfaces in the instance class base_init
>  - Need to test ordering as well as simply getting all the
>    interface_init and base_init functions called exactly once.

> Regards,
> 						Owen
>

---
ciaoTJ