From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39691) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDS4x-0005Qt-7B for qemu-devel@nongnu.org; Thu, 16 Jun 2016 03:51:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bDS4r-0007Jr-Ag for qemu-devel@nongnu.org; Thu, 16 Jun 2016 03:51:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52817) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDS4r-0007Jn-2K for qemu-devel@nongnu.org; Thu, 16 Jun 2016 03:51:01 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9B21BC04B300 for ; Thu, 16 Jun 2016 07:51:00 +0000 (UTC) From: Markus Armbruster References: <1466023310-13221-1-git-send-email-armbru@redhat.com> <1466023310-13221-3-git-send-email-armbru@redhat.com> <5761E9C8.3040507@redhat.com> Date: Thu, 16 Jun 2016 09:50:58 +0200 In-Reply-To: <5761E9C8.3040507@redhat.com> (Eric Blake's message of "Wed, 15 Jun 2016 17:50:32 -0600") Message-ID: <87ziqlefml.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH RFC 2/4] range: Eliminate direct Range member access List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, pbonzini@redhat.com, mst@redhat.com Eric Blake writes: > On 06/15/2016 02:41 PM, Markus Armbruster wrote: >> Users of struct Range mess liberally with its members, which makes >> refactoring hard. Create a set of methods, and convert all users to >> call them instead of accessing members. The methods have carefully >> worded contracts, and use assertions to check them. >> >> To help with tracking down the places that access members of struct >> Range directly, hide the implementation of struct Range outside of >> range.c by trickery. The trickery will be dropped in the next commit. > > Can't we just make Range an opaque type, and move its definition into > range.c? Oh, except we have inline functions that would no longer be > inline, unless we also had a range-impl.h private header. I'm not sure the inline functions are warranted, but I'd rather not debate that right now. So this patch makes Range kind-of opaque temporarily, by renaming its members outside range.c, just to help me find their users, and to make it more obvious to you that I found them all. >> Signed-off-by: Markus Armbruster >> --- > > >> @@ -2015,7 +2017,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, >> 0, 0x000A0000, 0x000BFFFF, 0, 0x00020000)); >> >> crs_replace_with_free_ranges(mem_ranges, >> - pci_hole->begin, pci_hole->end - 1); >> + range_lob(pci_hole), >> + range_upb(pci_hole)); > > Changes like this are nice, isolating us from what the actual struct stores. > > >> +++ b/include/qemu/range.h >> @@ -30,18 +30,110 @@ >> * - this can not represent a full 0 to ~0x0LL range. >> */ >> >> +bool range_is_empty(Range *range); >> +bool range_contains(Range *range, uint64_t val); >> +void range_make_empty(Range *range); >> +void range_set_bounds(Range *range, uint64_t lob, uint64_t upb); >> +void range_set_bounds1(Range *range, uint64_t lob, uint64_t upb_plus1); >> +uint64_t range_lob(Range *range); >> +uint64_t range_upb(Range *range); >> +void range_extend(Range *range, Range *extend_by); >> +#ifdef RANGE_IMPL >> + >> /* A structure representing a range of addresses. */ >> struct Range { >> uint64_t begin; /* First byte of the range, or 0 if empty. */ >> uint64_t end; /* 1 + the last byte. 0 if range empty or ends at ~0x0LL. */ >> }; >> >> +static inline void range_invariant(Range *range) >> +{ >> + assert((!range->begin && !range->end) /* empty */ >> + || range->begin <= range->end - 1); /* non-empty */ >> +} >> + >> +#define static >> +#define inline > > Yeah, that's a bit of a hack. I can live with it though. > > The other alternative is to squash 2 and 3 into a single patch on > commit; but having them separate was a nice review aid. I'm quite willing to squash if my trickery is considered too gross to be preserved for posterity. My personal preference is not to squash, for a full record. > Reviewed-by: Eric Blake Thanks!