>+ int64 res,resultlen;
>It's better to have them on separate lines.
Sorry for that, done.
>-unsigned
>+int64
> hex_decode(const char *src, unsigned len, char *dst)
>Do we want to explicitly cast the return value to int64? Will build on some platform crib if not done so?
>I don't know of such a platform but my knowledge in this area is not great.
I think current change can make sure nothing wrong.
>+ byteNo = (int)(n / 8);
>+ bitNo = (int)(n % 8);
>some comment explaining why this downcasting is safe here?
Done
>- proname => 'get_bit', prorettype => 'int4', proargtypes => 'bytea int4',
>+ proname => 'get_bit', prorettype => 'int4', proargtypes => 'bytea int8',
> prosrc => 'byteaGetBit' },
> { oid => '724', descr => 'set bit',
>- proname => 'set_bit', prorettype => 'bytea', proargtypes => 'bytea int4 int4',
>+ proname => 'set_bit', prorettype => 'bytea', proargtypes => 'bytea int8 int4',
> prosrc => 'byteaSetBit' },
>Shouldn't we have similar changes for following entries as well?
>{ oid => '3032', descr => 'get bit',
> proname => 'get_bit', prorettype => 'int4', proargtypes => 'bit int4',
> prosrc => 'bitgetbit' },
>{ oid => '3033', descr => 'set bit',
> proname => 'set_bit', prorettype => 'bit', proargtypes => 'bit int4 int4',
> prosrc => 'bitsetbit' },
Because 'bitsetbit' and 'bitgetbit' do not have to calculate bit size by 'multiply 8',
so I think it seems need not to change it.
>The tests you have added are for bytea variant which ultimately calles byteaGet/SetBit().
>But I think we also need tests for bit variants which will ultimately call bitgetbit and bitsetbit functions.
As above, it need not to touch 'bitgetbit' and 'bitsetbit'.
>Once you address these comments, I think the patch is good for a committer.
>So please mark the commitfest entry as such when you post the next version of patch.
Thanks a lot for the detailed review again, and changed patch attached.
