Re: [PATCH rc5-rt2 3/3] plist: convert the code to newimplementation

[Oleg Nesterov <oleg@tv-sign.ru>]

Daniel Walker wrote:
> 
> I think firstly, if you want to have success with this patch you'll need
> to clean it up a bit. I'm not an authority on clean code , but below
> isn't clean to my eyes.

Cleanups are always good, I am open to any suggestions.

>                         However, this is cleaner than your last
> attempt .

Thanks, but it was NOT changed from my last attempt. Just rediff.

> Hard coding MAX_PRIO isn't really acceptable.

I don't do that? Could you clarify?

>                                               If your going to make it
> more similar to list_head , why not name it plist_head instead of
> pl_head that way it's easy to switch between them.

I don't mind to rename, probably plist_head is better. I'd like to know
Ingo's opinion first.

>                                                              Like you
> remove a lot of the API which makes it less similar to a regular list .

For example?

> Also, making any changes to the internals of the plist structure outside
> of plist.c (or similar) isn't acceptable. For instance you set the node
> priority in several places, that should be hidden inside another
> function or macro. That makes it easier for people to change the
> internal structure without treading though tons of code.

Agreed, I already thought it makes sense to add plist_add_prio() helper.
Note that ->prio is set directly mostly right before plist_add() call.

> Changing plist_empty() doesn't make any sense to me.

plist_empty(head) means this list empty. plist_unhashed(node) means
this node is not on list. 

>                                                       Also changing
> dp_node to prio_list doesn't make much sense either.

Again, this patch is unchanged. I don't mind to rename, but recall
that it was sent before the current implementation become functional.
And honestly I don't like 'sp_node', this name is misleading, and
reflects first buggy implementation. It should be called 'all_nodes'
or something like this.

Oleg.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/