Eric Blake <ebl...@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-sta...@nongnu.org
> Signed-off-by: Eric Blake <ebl...@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.
