On Wed, Mar 09, 2016 at 12:02:38AM +0000, Rodriguez Betancourt, Esteban wrote: > Skiplist implementation intended for the IDL compound indexes > feature. > > Signed-off-by: Esteban Rodriguez Betancourt <esteb...@hpe.com>
Thanks for the patch. This is a new and nontrivial data structure, but the implementation does not include any correctness tests. Please write some. The 'probability' member is a constant, so I'm not sure why it's a member at all. I also don't understand why it's 64-bit. skiplist_insert() and skiplist_delete() both zero a local 520-byte (on 64-bit) array. It's expensive, and I don't think that's necessary. The comment on skiplist_destroy() says that it does not free the data, but the implementation calls the nodes' free_func, which I think does free the data. All the casts to "void *" in this file seem to be for removing const-ness. Please use CONST_CAST instead of plain casts, as a form of documentation. I'm surprised that there is no function to delete a skiplist_node directly, without internally performing another search. I'm surprised that there is no SKIPLIST_FOR_EACH_SAFE macro. Please use xmalloc() instead of malloc(). There are lots of minor violations of coding style. The commonest ones are that "if" and "while" should be followed by a space, and that pointers should be written as "type *" instead of "type*", but I see others. I recommend reading CodingStyle.md. Thanks, Ben. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
