Re: [Qemu-devel] [PATCH RFC 2/4] range: Eliminate direct Range member access

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
>> ---
>
>
>> @@ -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 <address@hidden>

Thanks!