Re: [coreboot] dt compiler patch

[Myles Watson]

On Tue, Nov 25, 2008 at 9:22 AM, ron minnich  wrote:
> On Tue, Nov 25, 2008 at 8:19 AM, Myles Watson  wrote:
> > > > > +/* we should only ever delete simple nodes: nodes with no kids. */
> > > > > +void del_node(struct node *node)
> > > > > +{
> > > > > struct node *n;
> > > > > assert(! node->children);
> > > > > if (first_node == node) {
> > > > >         first_node = node->next;
> > > > >         free(node);
> > > > >         return;
> > > > > }
> > > > > for_all_nodes(n) {
> > > > >         if (n->next != node)
> > > > >                 continue;
> > > > >         n->next = node->next;
> > > > >         n->next_sibling = node->next_sibling;
> > > >
> > > > Will this always be true?  It seems like you need to go through again
>
> to
> > > do
> > > > the sibling links right.
> > >
> > > I don't think so because, at this point, n is prev(node to be deleted).
> > >
> > > so I am setting prev(node)->next and next_sibling to node->next etc.
> > > Am I missing something?
> >
> > I was thinking about the case where n wasn't the sibling of the node
>
> being
> > deleted.  For example, a first child being deleted.
> >
> > It seems like you should at least check that n->next_sibling was node
>
> before
> > setting it to node->next_sibling.
>
> I have code whiteout, can you write this correctly for me :-)

I don't know if it's correct, but here's a first try:

/* we should only ever delete simple nodes: nodes with no kids. */
void del_node(struct node *node)
{
    struct node *n, *s;
    assert(! node->children);
    if (first_node == node) {
        first_node = node->next;
        free(node);
        return;
    }

/* If the node is the first child, set parent->children link. */
    if (node->parent->children == node)
        node->parent->children = node->next_sibling;
    else {
        /* Remove from sibling list. */
        for (n=node->parent->children; n->next_sibling;
             n=n->next_sibling)
            if (n->next_sibling == node)
                break;
        n->next_sibling = node->next_sibling;
    }

for_all_nodes(n) {
        if (n->next != node)
            continue;
        n->next = node->next;
        if (node == last_node)
            last_node = n;
        break;
    }
    free(node);
}

It doesn't check to see if the child is really a child of its parent.  I
figured that was a given.
> > > > Do we guarantee that you will never be appending to a NULL list (first
> > >
> > > ==
> > > > NULL), I didn't see that check.
> > >
> > > Here's my thinking on this. The guarantee is that there is always a
> > > root node -- we don't ever remove that.
> >
> > I guess I'm confused here.  I was talking about when we call it from
> > fixup_properties with chipnodes as the list.  It looks like we could
>
> delete
> > all of them.  Do I have it backward?
>
> yes. We should only ever delete mainboard nodes, not chip nodes.

Right.

Thanks,

Myles