Hi Colinm
On Tue, Apr 20, 2010 at 2:08 PM, Colin McDonald
<[email protected]> wrote:
> No, actually the first argument to compare_node_id2 is the address of
> a user_id, not a node_id. It took me a while to spot it. You can see
> this in the call to bsearch in kfdata_read:
>
> parent = *(Lib3dsNode**)bsearch(&p->user_id, nodes, num_nodes,
> sizeof(Lib3dsNode*), compare_node_id2);
>
> user_id is an unsigned int (declared in lib3ds.h), so my change to the
> cast of argument a in compare_node_id2 is required: (unsigned*)a. It
> doesn't work on a big-endian system otherwise. It's a bit unusual to
> have different types for argument a & b in the comparison routine, but
> is legitimate for bsearch.
Ahhh, I assumed the first argument of the same type as the node_id
that comparison was being done against. It's really odd that the sort
was done against the node_id then the binary search done against the
user_id when both are of different types. This code feels really
odd. I'm reading the block of code and struggling to work out what
it's doing:
{
Lib3dsNode **nodes = (Lib3dsNode **)malloc(num_nodes *
sizeof(Lib3dsNode*));
unsigned i;
Lib3dsNode *p, *q, *parent;
p = file->nodes;
for (i = 0; i < num_nodes; ++i) {
nodes[i] = p;
p = p->next;
}
qsort(nodes, num_nodes, sizeof(Lib3dsNode*), compare_node_id);
p = last;
while (p) {
q = (Lib3dsNode *)p->user_ptr;
if (p->user_id != 65535) {
parent = *(Lib3dsNode**)bsearch(&p->user_id, nodes,
num_nodes, sizeof(Lib3dsNode*), compare_node_id2);
if (parent) {
q->next = p->next;
p->next = parent->childs;
p->parent = parent;
parent->childs = p;
} else {
/* TODO: warning */
}
}
p->user_id = 0;
p->user_ptr = NULL;
p = q;
}
free(nodes);
}
The fact that the types of node_id and user_id are different raises
alarm bells to me. I wonder if it would be worth contact the
maintainer of lib3ds to clarify what the actual code should be, it
might be that user_id is being misused here, or that the type is
simply wrong.
I've just merged your change for replacing the first parameters type
from being unsigned short to unsigned, and have also added a comment
explaining this. This is now checked in.
While checking this in does now that compare_node_id2 is correctly
typed w.r.t to the usage in the bsearch call, I do still feel very
uneasy about the validity of the calling code/types being mixed.
Robert.
_______________________________________________
osg-submissions mailing list
[email protected]
http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org
