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

[Jacek Caban]

On 15.02.2017 23:42, Henri Verbeet wrote:
> On 15 February 2017 at 17:46, Sebastian Lackner  wrote:
> > Wouldn't it make more sense to integrate the clearing directly into the macro
> > somehow, if its only used for destructors? You could for example reset ->root
> > immediately after initializating the cursor. Otherwise, if the macro is supposed
> > to be a general purpose postorder iterator, it would make more sense to use
> > a different name.
>
> Issues aside, and arguably this is personal taste, etc., but I think
> the premise of the patch is questionable.

Please keep populist simplifications aside as well.
> 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. 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);

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.

Cheers,
Jacek