[m-rev.] for review: fix some error reporting in the bitmap module
jfischer at opturion.com
Fri Dec 29 14:58:17 AEDT 2017
On Fri, 29 Dec 2017, Zoltan Somogyi wrote:
>> +throw_byte_bounds_error(BM, Pred, ByteIndex) :-
>> + ( if BM ^ num_bits = 0 then
>> + UB = -1
>> + else
>> + UB = BM ^ num_bits / bits_per_byte
>> + ),
>> + string.format(
>> + "%s: byte index %d is out of bounds [0, %d].",
>> + [s(Pred), i(ByteIndex), i(UB)], Msg),
>> + throw_bitmap_error(Msg).
> I think that final ] can look wrong. For example, ...
>> --- a/tests/hard_coded/bitmap_bytes.exp
>> +++ b/tests/hard_coded/bitmap_bytes.exp
>> @@ -0,0 +1,19 @@
>> +^ byte(-1): bitmap.byte: byte index -1 is out of bounds [0, -1].
>> +^ byte(0): bitmap.byte: byte index 0 is out of bounds [0, -1].
>> +^ byte(1): bitmap.byte: byte index 1 is out of bounds [0, -1].
>> +Bitmap: 00000000.00000000.00000000
>> +^ byte(-1): bitmap.byte: byte index -1 is out of bounds [0, 3].
>> +^ byte(0): 0
>> +^ byte(1): 0
>> +^ byte(2): 0
>> +^ byte(3): bitmap.byte: byte index 3 is out of bounds [0, 3].
> ... having 3 being out of bounds in [0, 3] looks wrong,
> while it would be understandable if the bounds were [0, 3).
The final character should be ")"; I've fixed it.
> The issue here is: what should the error message say if the
> bitmap contains the first bit of the selected byte, but is missing
> some later bits? In that case, both of the obvious options
Partial final bytes are (and always have been) treated as being out of
bounds. This isn't stated in the documentation (which I will fix in my
next change), but it is what the implmentation has always done. (It is
consistent with the way the rest of this module treats byte oriented
requests on bitmaps with partial final bytes.)
I'll deal with the other points raised in your review in a separate
To step back a bit: a lot of the complication in this module is caused
by it trying to do two things, (1) be a map from int -> bool and (2) act
as a container for bytes; the fact that partial final bytes are possible
complicates (2) quite a bit.
More information about the reviews