[LITMUS^RT] Binary heaps for Litmus
Björn Brandenburg
bbb at mpi-sws.org
Fri Mar 30 16:48:16 CEST 2012
On Mar 22, 2012, at 12:29 AM, Glenn Elliott wrote:
> I realized that binheap_delete() was more complicated than necessary. Cleaned that up and then added binheap_decrease(). Sorry for the explosion of patches.
>
The attached patches are difficult to comment on. I'll copy and paste some segments.
>
> 365 /* NULL data pointers are used internally */
> 366 if(!data) {
> 367 WARN();
> 368 return;
> 369 }
Should that be a BUG_ON(!data)? It's clearly a misuse of the API, so why only a WARN()? It's not like the user could be expected to do anything about this. A BUG_ON() at least makes sure that it's noticed.
> 422 static inline void __binheap_delete_root(struct binheap_handle *handle,
> 423 struct binheap_node *container)
>
> 500 static inline void __binheap_delete(
> 501 struct binheap_node *node_to_delete,
> 502 struct binheap_node *user_of_node,
> 503 struct binheap_handle *handle)
> 504
>
I don't know what your TAB setting is, but the above code looks very strangely indented to me (same in cgit).
I'm also not quite sure what replacing the binomial heap in GSN-EDF with a binary heap is buying us. The new code still relies on dynamic memory allocation and does not look much simpler to me than the existing binomial heap implementation. Is there some pressing need? I'm not really opposed to it, but I'd like to understand the motivation.
Anyway, before this is merged, I think it should be rebased to hide the fixup commits. (Commit bdce67bc2babc2e5b3b2440964e9cf819ac814dc contains unrelated changes, for example).
Thanks,
Björn
More information about the litmus-dev
mailing list