| Message ID | 1463458137-19109-2-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com> > --- > 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.
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; + } +} + +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) { 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 diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c index 30b5879..b546e5f 100644 --- a/qapi/string-input-visitor.c +++ b/qapi/string-input-visitor.c @@ -61,8 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) cur = g_malloc0(sizeof(*cur)); cur->begin = start; cur->end = start + 1; - siv->ranges = g_list_insert_sorted_merged(siv->ranges, cur, - range_compare); + siv->ranges = range_list_insert(siv->ranges, cur); cur = NULL; str = NULL; } else if (*endptr == '-') { @@ -76,10 +75,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) cur = g_malloc0(sizeof(*cur)); cur->begin = start; cur->end = end + 1; - siv->ranges = - g_list_insert_sorted_merged(siv->ranges, - cur, - range_compare); + siv->ranges = range_list_insert(siv->ranges, cur); cur = NULL; str = NULL; } else if (*endptr == ',') { @@ -87,10 +83,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) cur = g_malloc0(sizeof(*cur)); cur->begin = start; cur->end = end + 1; - siv->ranges = - g_list_insert_sorted_merged(siv->ranges, - cur, - range_compare); + siv->ranges = range_list_insert(siv->ranges, cur); cur = NULL; } else { goto error; @@ -103,9 +96,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) cur = g_malloc0(sizeof(*cur)); cur->begin = start; cur->end = start + 1; - siv->ranges = g_list_insert_sorted_merged(siv->ranges, - cur, - range_compare); + siv->ranges = range_list_insert(siv->ranges, cur); cur = NULL; } else { goto error; diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c index d013196..5ea395a 100644 --- a/qapi/string-output-visitor.c +++ b/qapi/string-output-visitor.c @@ -85,7 +85,7 @@ static void string_output_append(StringOutputVisitor *sov, int64_t a) Range *r = g_malloc0(sizeof(*r)); r->begin = a; r->end = a + 1; - sov->ranges = g_list_insert_sorted_merged(sov->ranges, r, range_compare); + sov->ranges = range_list_insert(sov->ranges, r); } static void string_output_append_range(StringOutputVisitor *sov, @@ -94,7 +94,7 @@ static void string_output_append_range(StringOutputVisitor *sov, Range *r = g_malloc0(sizeof(*r)); r->begin = s; r->end = e + 1; - sov->ranges = g_list_insert_sorted_merged(sov->ranges, r, range_compare); + sov->ranges = range_list_insert(sov->ranges, r); } static void format_string(StringOutputVisitor *sov, Range *r, bool next,
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 <eblake@redhat.com> --- include/qemu/range.h | 49 +++++++++++++++++++------------------------- qapi/string-input-visitor.c | 17 ++++----------- qapi/string-output-visitor.c | 4 ++-- 3 files changed, 27 insertions(+), 43 deletions(-)