Re: [Qemu-devel] [PATCH 1/2] qapi: Simplify use of range.h

[Markus Armbruster <armbru@redhat.com>]

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.