range_deserialize + range_get_flags - Mailing list pgsql-hackers

From Alvaro Herrera
Subject range_deserialize + range_get_flags
Date
Msg-id 20200306200343.GA625@alvherre.pgsql
Whole thread Raw
List pgsql-hackers
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

Attachment

pgsql-hackers by date:

Previous
From: Chapman Flack
Date:
Subject: Re: Unicode escapes with any backend encoding
Next
From: Tom Lane
Date:
Subject: Re: More tests to stress directly checksum_impl.h