From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49284) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b7i6o-0005yB-BO for qemu-devel@nongnu.org; Tue, 31 May 2016 07:45:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b7i6i-0002a5-Bd for qemu-devel@nongnu.org; Tue, 31 May 2016 07:45:17 -0400 From: Markus Armbruster References: <1463458137-19109-1-git-send-email-eblake@redhat.com> <1463458137-19109-2-git-send-email-eblake@redhat.com> Date: Tue, 31 May 2016 13:45:08 +0200 In-Reply-To: <1463458137-19109-2-git-send-email-eblake@redhat.com> (Eric Blake's message of "Mon, 16 May 2016 22:08:56 -0600") Message-ID: <87pos2fo3f.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 1/2] qapi: Simplify use of range.h List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Michael Roth , qemu-stable@nongnu.org Eric Blake writes: > Calling our function g_list_insert_sorted_merged() is a misnomer, > since we are NOT writing a glib function. Furthermore, we are > making every caller pass the same comparator function of > range_merge(): any caller that does otherwise would break > in weird ways since our internal call to ranges_can_merge() is > hard-coded to operate only on ranges, rather than paying > attention to the caller's comparator. > > Better is to fix things so that callers don't have to care about > our internal comparator, and to pick a function name that makes > it obvious that we are operating specifically on a list of ranges > and not a generic list. Plus, refactoring the code here will > make it easier to plug a memory leak in the next patch. > > Note that no one used the return value of range_merge(), and that > it can only be called if its precondition was satisfied, so > simplify that while in the area. > > The declaration of range_compare() had to be hoisted earlier > in the file. > > CC: qemu-stable@nongnu.org > Signed-off-by: Eric Blake > --- > include/qemu/range.h | 49 +++++++++++++++++++------------------------- > qapi/string-input-visitor.c | 17 ++++----------- > qapi/string-output-visitor.c | 4 ++-- > 3 files changed, 27 insertions(+), 43 deletions(-) > > diff --git a/include/qemu/range.h b/include/qemu/range.h > index c903eb5..4a4801b 100644 > --- a/include/qemu/range.h > +++ b/include/qemu/range.h > @@ -65,30 +65,36 @@ static inline bool ranges_can_merge(Range *range1, Range *range2) > return !(range1->end < range2->begin || range2->end < range1->begin); > } > > -static inline int range_merge(Range *range1, Range *range2) > +static inline void range_merge(Range *range1, Range *range2) > { > - if (ranges_can_merge(range1, range2)) { > - if (range1->end < range2->end) { > - range1->end = range2->end; > - } > - if (range1->begin > range2->begin) { > - range1->begin = range2->begin; > - } > + if (range1->end < range2->end) { > + range1->end = range2->end; > + } > + if (range1->begin > range2->begin) { > + range1->begin = range2->begin; > + } > +} Why can the conditional be dropped? The commit message doesn't quite explain. > + > +static inline gint range_compare(gconstpointer a, gconstpointer b) > +{ > + Range *ra = (Range *)a, *rb = (Range *)b; > + if (ra->begin == rb->begin && ra->end == rb->end) { > return 0; > + } else if (range_get_last(ra->begin, ra->end) < > + range_get_last(rb->begin, rb->end)) { > + return -1; > + } else { > + return 1; > } > - > - return -1; > } > > -static inline GList *g_list_insert_sorted_merged(GList *list, > - gpointer data, > - GCompareFunc func) > +static inline GList *range_list_insert(GList *list, Range *data) Cleans up gratuitous use of void *. Would you like to mention this in your commit message? > { > GList *l, *next = NULL; > Range *r, *nextr; > > if (!list) { > - list = g_list_insert_sorted(list, data, func); > + list = g_list_insert_sorted(list, data, range_compare); > return list; > } > > @@ -111,23 +117,10 @@ static inline GList *g_list_insert_sorted_merged(GList *list, > } > > if (!l) { > - list = g_list_insert_sorted(list, data, func); > + list = g_list_insert_sorted(list, data, range_compare); > } > > return list; > } > > -static inline gint range_compare(gconstpointer a, gconstpointer b) > -{ > - Range *ra = (Range *)a, *rb = (Range *)b; > - if (ra->begin == rb->begin && ra->end == rb->end) { > - return 0; > - } else if (range_get_last(ra->begin, ra->end) < > - range_get_last(rb->begin, rb->end)) { > - return -1; > - } else { > - return 1; > - } > -} > - > #endif [...] Why on earth does this code live in a header? Let's move at least range_list_insert() to util/range.c. Moving it in PATCH 3/2 would be fine.