2018-03-12 9:08 GMT+01:00 Anthony Bykov <a.by...@postgrespro.ru>: > On Mon, 5 Mar 2018 14:03:37 +0100 > Pavel Stehule <pavel.steh...@gmail.com> wrote: > > > Hi > > > > I am looking on this patch. I found few issues: > > > > 1. compile warning > > > > I../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 > > -I/usr/lib64/perl5/CORE -c -o jsonb_plperl.o jsonb_plperl.c > > jsonb_plperl.c: In function ‘SV_FromJsonbValue’: > > jsonb_plperl.c:69:9: warning: ‘result’ may be used uninitialized in > > this function [-Wmaybe-uninitialized] > > return result; > > ^~~~~~ > > jsonb_plperl.c: In function ‘SV_FromJsonb’: > > jsonb_plperl.c:142:9: warning: ‘result’ may be used uninitialized in > > this function [-Wmaybe-uninitialized] > > return result; > > ^~~~~~ > > > > 2. bad comment > > > > /* > > * SV_ToJsonbValue > > * > > * Transform Jsonb into SV --- propably reverse direction > > */ > > > > > > /* > > * HV_ToJsonbValue > > * > > * Transform Jsonb into SV > > */ > > > > /* > > * plperl_to_jsonb(SV *in) > > * > > * Transform Jsonb into SV > > */ > > > > 3. Do we need two identical tests fro PLPerl and PLPerlu? Why? > > > > Regards > > > > Pavel > > Hello, thanks for reviewing my patch! I really appreciate it. > > That warnings are created on purpose - I was trying to prevent the case > when new types are added into pl/perl, but new transform logic was not. > Maybe there is a better way to do it, but right now I'll just add the > "default: pg_unreachable" logic. > > About the 3 point - I thought that plperlu and plperl uses different > interpreters. And if they act identically on same examples - there > is no need in identical tests for them indeed. >
plperlu and plperl uses same interprets - so the duplicate tests has not any sense. But in last versions there are duplicate tests again The naming convention of functions is not consistent almost are are src_to_dest This is different and it is little bit messy +static SV * +SV_FromJsonb(JsonbContainer *jsonb) This comment is broken +/* + * plperl_to_jsonb(SV *in) + * + * Transform Jsonb into SV ---< should be SV to Jsonb + */ +PG_FUNCTION_INFO_V1(plperl_to_jsonb); +Datum Regards Pavel > > Point 2 is fixed in this version of the patch. > > Please, find in attachments a new version of the patch. > > -- > Anthony Bykov > Postgres Professional: http://www.postgrespro.com > The Russian Postgres Company >
