Hi 2018-03-13 15:50 GMT+01:00 Nikita Glukhov <n.glu...@postgrespro.ru>:
> Hi. > > I have reviewed this patch too. Attached new version with v8-v9 delta-patch. > > Here is my changes: > > * HV_ToJsonbValue(): > - addded missing hv_iterinit() > - used hv_iternextsv() instead of hv_iternext(), HeSVKEY_force(), HeVAL() > > * SV_ToJsonbValue(): > - added recursive dereferencing for all SV types > - removed unnecessary JsonbValue heap-allocations > > * Jsonb_ToSV(): > - added iteration to the end of iterator needed for correct freeing of > JsonbIterators > > * passed JsonbParseState ** to XX_ToJsonbValue() functions. > > * fixed warnings (see below) > > * fixed comments (see below) > > > Also I am not sure if we need to use newRV() for returning SVs in > Jsonb_ToSV() and JsonbValue_ToSV(). > > > On 12.03.2018 18:06, Pavel Stehule wrote: > > 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; >> > ^~~~~~ >> >> 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. >> >> pg_unreachable() is replaced with elog(ERROR) for reporting impossible > JsonbValue types and JsonbIteratorTokens. > > > 3. Do we need two identical tests fro PLPerl and PLPerlu? Why? >> > >> > Regards >> > >> > Pavel >> >> 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 > > I have not removed duplicate test yet, because I am not sure that this > test does not make sense at all. > ok .. the commiter can decide it > > 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) > > Renamed to Jsonb_ToSV() and JsonbValue_ToSV(). > > 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 > > Fixed. > It looks well the patch has tests and doc, there are not any warnings or compilation issues all tests passed I'll mark this patch as ready for commiter Regards Pavel > > > -- > Nikita Glukhov > Postgres Professional: http://www.postgrespro.com > The Russian Postgres Company >
