I noticed a weird thing about rangetypes API while reviewing multiranges
-- the combination of range_deserialize immediately followed by
range_get_flags. It seems quite odd to have range_deserialize return
only one flag (empty) and force callers to use a separate
range_get_flags call in order to fetch anything else they need. I
propose that it's simpler to have range_deserialize have an out param
for flags (replacing "empty"), and callers can examine "IsEmpty" from
that using a macro accessor. So there are two macros now:
RangeFlagsIsEmpty() takes the 'flags' value and return whether the bit
is set. Its companion RangeIsEmpty() does the range_get_flags() dance.
The attached patch does that, with a net savings of 8 lines of code in
rangetypes.c. I know, it's not amazing. But it's slightly cleaner this
way IMO.
The reason things are this way is that initially (commit 4429f6a9e3e1)
were all opaque; the external observer could only see "empty" when
deserializing the value. Then commit 37ee4b75db8f added
range_get_flags() to obtain the flags from a range, but at that point it
was only used in places that did not deserialized the range anyway, so
it was okay. I think it was commit c66e4f138b04 that ended up having
both deserialize and get_flags in succession. So things are weird now,
but they have not always been like that.
I also chose to represent the flags out param as uint8 in
range_deserialize. With this change, the compiler warns for callers
using the old API (it used to take bool *), giving a chance to update.
--
Álvaro Herrera
