On Mon, 5 Nov 2012 13:41:06 +0000, Ben Widawsky <ben at bwidawsk.net> wrote: > On Fri, 19 Oct 2012 18:03:11 +0100 > Chris Wilson <chris at chris-wilson.co.uk> wrote: > > - BUG_ON(drm_mm_hole_node_start(node) > > - != drm_mm_hole_node_end(node)); > > + BUG_ON(node->start + node->size != > > + list_entry(node->node_list.next, > > + struct drm_mm_node, node_list)->start); > > + > > > > This seems harder to read to me than before. I suspect you've changed it > only because you've moved the inline functions down, but I also don't > understand the reason for that. Since I move the BUG_ON() into drm_mm_hole_node_end() and this is the atypical caller, I chose to make this code less clear in order to make the other callsites clearer. The other choice is to use __drm_mm_hole_node_end(), which is probably worthwhile. > > + > > +/* Note that we need to unroll list_for_each_entry in order to inline > > + * setting hole_start and hole_end on each iteration and keep the > > + * macro sane. > > + */ > > This comment is probably better suited for the commit message then here > where it'd require git blame to make any sense of it. No, I think list_for_each_entry() is a common macro that the reader might have expected to see and so we need to explain why we cannot use it here. -Chris -- Chris Wilson, Intel Open Source Technology Centre
