Woops, forgot to copy the list.
On Sat, Dec 12, 2009 at 2:15 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sat, Dec 12, 2009 at 2:00 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Roman Kononov <kononov(at)ftml(dot)net> writes:
>>> The bitfromint8() and bitfromint4() are hosed. They produce wrong
>>> results when the BIT size is more than 64 and 32 respectively, and the
>>> BIT size is not multiple of 8, and the most significant byte of the
>>> integer value is not 0x00 or 0xff.
>>
>> Hm, you're right, it's definitely busted ...
>>
>>> The patch re-implements the conversion functions.
>>
>> ... but I don't much care for this patch. It's unreadable, uncommented,
>> and doesn't even try to follow Postgres coding conventions.
>>
>> It looks to me like the actual bug is that the "first fractional byte"
>> case ought to shift by destbitsleft - 8 not srcbitsleft - 8. A
>> secondary problem (which your patch fails to fix) is that if the
>> compiler chooses to implement signed >> as zero-fill rather than
>> sign-bit-fill (which it is allowed to do per C99) then we'll get
>> wrong, or at least not the desired, results. So I arrive at
>> the attached patch.
>
> I'm not sure this fixes it, although I haven't tested. When we take
> the /* store first fractional byte */ branch, destbitsleft is between
> 1 and 7 bits greater than srcbitsleft. We then subtract 8 from
> destbitsleft, and the comment on the next line now asserts that the
> two are equal. That doesn't seem right.
>
> Also, I thought about the sign extension problem, but aren't we
> chopping those bits off anyway on the next line?
>
> ...Robert
>