Re: [Xen-devel] [PATCH] fix page_list_splice()

[Keir Fraser <keir.xen@xxxxxxxxx>]

On 07/06/2012 20:10, "Jan Beulich" <jbeulich@xxxxxxxx> wrote:

>> I mean to replace the comment with a second assertion, not to replace the
>> assertion your patch already adds. Does that make sense?
> 
> From the perspective of the operation here, we really just want to make
> sure that the new list head's prev points to where the old one's pointed;
> the fact that they're both supposed to be PAGE_LIST_NULL doesn't really
> matter. Hence simply asserting that the assignment is superfluous seems
> the best choice to me, with the comment explaining why. If you have
> suggestions for better wording of the comment, I'll gladly take those.
> 
> The second best choice imo would be to just have the superfluous
> assignment there (making it likely that later someone will come and
> ask why it's there when not really needed).

Well, I already did check in a version with two assertions, yesterday. :) I
like assertions, and asserting things about the good form of the data
structure being manipulated, even if that good form is not essential to this
specific operation, is not necessarily bad. Especially while we're already
asserting other things about those same list pointers.

It's not a major point here of course, it just seemed silly to to me to take
up a source line with a comment which could be stated just as clearly with
an assertion, and with added benefit (imo).

 -- Keir

> Jan
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel