Leonardo F wrote:
>> In the documentation, the get_bit and set_bit methods are added
>> to a list where we state "The following SQL-standard functions
>> work on bit strings as well as character strings"; however they
>> are not SQL-standard functions and are implemented on binary
>> strings, not character strings.
>
> Ok, I didn't change that, but I guess I can fix it.
The items already on that list, and substring function that you
added, *do* fit the description; get_bit and set_bit don't fit that
description, so they don't belong on that list. They must be
described in some other way.
>> There is a peculiar omission from the patch -- it adds support
>> for the overlay function to bit strings but not binary strings.
>> Since the standard drops the bit string as a standard type in the
>> 2003 standard, in favor of boolean type and binary string types,
>> it seems odd to omit binary string support (which is defined in
>> the standard) but include bit string support (which isn't). What
>> the patch adds seems useful, and I don't think the omission
>> should be considered fatal, but it would be nice to see us also
>> cover binary strings if we can.
>
> Mmh, don't know how complicated that would be, but I can give it a
> try: I guess it could be a simple SQL replacement (as for char
> strings and bit strings?)
The standard explicitly defines it as being equivalent to the
substring and concatenation technique, so that should work.
>> Well, there was one thing which is beyond my ken in this regard:
>> I'm not sure that the cases from bits8 to (unsigned char *) are
>> needed or appropriate, although on my machine they don't seem to
>> hurt. Perhaps someone with a better grasp of such issues can
>> comment.
>
> That's some kind of copy&paste from varlena.c byteaGetBit: I don't
> know if they can be avoided.
Ah, but there it's casting the result of VARDATA_ANY. Here you
already have the value in a bits8 variable. I looked it over some
more and I'm pretty confident we can omit those casts in bitsetbit.
> [pointer is zero-based]
>
>> Ok; I found them bizarre too, but I kept it consistent.
>> [int32 used for bit values]
>
> Same as above: I tried to be consistent.
>> [ERRCODE_ARRAY_SUBSCRIPT_ERROR used when query has no array]
>
> Again, same as above: consistency; but I can change it if you want
I'm not suggesting the above require changes; I mentioned those
primarily for the benefit of whoever eventually commits this, to try
to save time chasing down the issues.
>> [questions use of oldByte and newByte local variables]
>
> I agree, you version is cleaner.
Cool. Should look even tidier without those casts. :-)
-Kevin