[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Keir Fraser <keir.xen@xxxxxxxxx>
  • Date: Fri, 08 Jun 2012 08:51:08 +0100
  • Cc: jisooy@xxxxxxxxx, xen-devel@xxxxxxxxxxxxx
  • Delivery-date: Fri, 08 Jun 2012 07:52:02 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xen.org>
  • Thread-index: Ac1FS3a0R9qsSdhHVUOnojMBMgdb2w==
  • Thread-topic: [Xen-devel] [PATCH] fix page_list_splice()

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.