[PATCH v2] winedevice: Use WINE_RB_FOR_EACH_ENTRY_DESTRUCTOR to free drivers in async_shutdown_drivers.

[Henri Verbeet]

On 16 February 2017 at 17:17, Jacek Caban <jacek at 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.