Eric Blake <ebl...@redhat.com> 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 <arm...@redhat.com> >> --- > > >> @@ -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 <ebl...@redhat.com> Thanks!