On 16 February 2017 at 17:17, Jacek Caban jacek@codeweavers.com wrote:
Please keep populist simplifications aside as well.
It seems this came over different than intended, so let's leave that behind us.
I.e., I don't think replacing
wine_rb_destroy( &wine_drivers, wine_drivers_rb_destroy, NULL );with
WINE_RB_FOR_EACH_ENTRY_DESTRUCTOR( driver, next, &wine_drivers,struct wine_driver, entry )
is an improvement to begin with.
This missing the point by skipping the context.
It does skip some context, but I do think it's the core of the argument.
Comparing those two single lines just doesn't make any sense. The second one just do way more things, while the first one needs separated callback to do them.
Proper comparison for proposed changes is:
void rb_entry_destroy( struct wine_rb_entry *entry, void *context ) {
struct some_struct *ptr = WINE_RB_ENTRY_VALUE( entry, struct some_struct entry ); heap_free(ptr);}
/* inside other function */ wine_rb_destroy( &some_rb_tree, rb_entry_destroy, NULL );
compared to just two lines:
WINE_RB_FOR_EACH_ENTRY_DESTRUCTOR( iter, next, &some_rb_tree, struct some_struct, entry ) heap_free(iter);
Sure. But then in the specific case here you also have the unload_driver() signature change which affects device_handler().
The above example is a case where you don't need pass the context. If you meant to say WINE_RB_FOR_EACH_ENTRY_DESTRUCTOR is not an improvement in general, then let's see at an example where you need a non-trivial context to free the entry:
struct free_entry_context { void *x; int y; };
void rb_entry_destroy( struct wine_rb_entry *entry, void *context ) { struct some_struct *ptr = WINE_RB_ENTRY_VALUE( entry, struct some_struct entry ); struct free_entry_context *ctx = context; free_entry(ptr, ctx->x, ctx->y); }
/* inside other function */ struct free_entry_context ctx; ctx.x = x; ctx.y = y wine_rb_destroy( &some_rb_tree, rb_entry_destroy, &ctx );
All the above code can be replaced with, again, just two lines:
WINE_RB_FOR_EACH_ENTRY_DESTRUCTOR( iter, next, &some_rb_tree, struct some_struct, entry ) free_entry(iter, x, y);
It's still a matter of taste, but I'm sure that no callback, no context mess, two line solution looks much better when you compare the whole thing, not just a random single line.
In that example, probably. Part of that is predicated on the context structure being something that you don't need to already have elsewhere in the code though.
To put all that a different way, I don't think WINE_RB_FOR_EACH_ENTRY_DESTRUCTOR is inherently worse than wine_rb_destroy(), but I don't think the reverse is true either. Personally, I dislike macros more than callbacks, while I suspect the reverse is true for you. Perhaps in a similar vein, I don't care much about line counts. I think pretty much all of those are largely personal preferences, and that's fine, but that does mean I think the choice of one over the other should be the choice of the respective maintainers.