Re: [PATCH v2] winedevice: Use WINE_RB_FOR_EACH_ENTRY_DESTRUCTOR to free drivers in async_shutdown_drivers.
On 15.02.2017 17:07, Jacek Caban wrote:
+ wine_rb_clear( &wine_drivers, NULL, NULL );
This fixes the issue, but passing a non-NULL callback to clear() would crash because the rbtree is temporarily corrupted. In all other places clear() is called to clear the content, not as a replacement for init(). 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.
On 15 February 2017 at 17:46, Sebastian Lackner <sebastian(a)fds-team.de> 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. 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.
On 15.02.2017 23:42, Henri Verbeet wrote:
On 15 February 2017 at 17:46, Sebastian Lackner <sebastian(a)fds-team.de> 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
On 16 February 2017 at 17:17, Jacek Caban <jacek(a)codeweavers.com> wrote:
On 15.02.2017 23:42, Henri Verbeet wrote:
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.
Well, that's a great way to kill any constructive discussion, and frankly lose my respect.
On 16 February 2017 at 17:17, Jacek Caban <jacek(a)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.
On 15.02.2017 17:46, Sebastian Lackner wrote:
On 15.02.2017 17:07, Jacek Caban wrote:
+ wine_rb_clear( &wine_drivers, NULL, NULL ); This fixes the issue, but passing a non-NULL callback to clear() would crash because the rbtree is temporarily corrupted. In all other places clear() is called to clear the content, not as a replacement for init().
Right. It not relevant here, but I actually think that wine_rb_clear should not have callback argument.
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.
I like that idea. I will revisit the patch later.
Otherwise, if the macro is supposed to be a general purpose postorder iterator, it would make more sense to use a different name.
No, it's not meant to be general purpose. I don't see any use case other than freeing elements. Other use cases are covered by ordered iterators. Thanks, Jacek
participants (3)
-
Henri Verbeet -
Jacek Caban -
Sebastian Lackner