>+ 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. Regards, Highgo Software (Canada/China/Pakistan) URL : www.highgo.ca EMAIL: mailto:movead(dot)li(at)highgo(dot)ca
long_bytea_string_bug_fix_ver5.patch
Description: Binary data