Re: Transform for pl/perl

On Thu, 7 Dec 2017 12:54:55 +0300
Anthony Bykov <a.bykov@postgrespro.ru> wrote:

> On Fri, 1 Dec 2017 15:49:21 -0300
> Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> 
> > A few very minor comments while quickly paging through:
> > 
> > 1. non-ASCII tests are going to cause problems in one platform or
> > another.  Please don't include that one.
> > 
> > 2. error messages
> >    a) please use ereport() not elog()
> >    b) conform to style guidelines: errmsg() start with lowercase,
> > others are complete phrases (start with uppercase, end with period)
> >    c) replace
> >       "The type you was trying to transform can't be represented in
> > JSONB" maybe with
> >       errmsg("could not transform to type \"%s\"", "jsonb"),
> >       errdetail("The type you are trying to transform can't be
> > represented in JSON") d) same errmsg() for the other error; figure
> > out suitable errdetail.
> > 
> > 3. whitespace: don't add newlines to while, DirectFunctionCallN,
> > pnstrdup.
> > 
> > 4. the "relocatability" test seems pointless to me.
> > 
> > 5. "#undef _" looks bogus.  Don't do it.
> >   
> 
> Hello,
> thank you for your time.
> 
> 1. I really think that it might be a good practice to test non ASCII
>   symbols on platforms where it is possible. Maybe locale-dependent
>   Makefile? I've already spent pretty much time trying to find
> possible solutions and I have no results. So, I've deleted this
> tests. Maybe there is a better solution I don't know about?
> 
> 2. Thank you for this one. Writing those errors were really pain for
>   me. I've changed those things in new patch.
> 
> 3. I've fixed all the whitespaces you was talking about in new version
>   of the patch.
> 
> 4. The relocatibility test is needed in order to check if patch is
>   still relocatable. With this test I've tried to prove the code
>   "relocatable=true" in *.control files. So, I've decided to leave
> them in next version of the patch.
> 
> 5. If I delete #undef _, I will get the warning:
> 	/usr/lib/x86_64-linux-gnu/perl/5.22/CORE/config.h:3094:0:
> 	warning: "_" redefined #define _(args) args
>  
> 	In file included from ../../src/include/postgres.h:47:0,
>                  from jsonb_plperl.c:12:
> 	../../src/include/c.h:971:0: note: this is the location of the
> 	previous definition #define _(x) gettext(x)
>   This #undef was meant to fix the warning. I hope a new comment next
>   to #undef contains all the explanations needed.
> 
> Please, find a new version of the patch in attachments to this
> message.
> 
> 
> --
> Anthony Bykov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company

Forgot the patch.

--
Anthony Bykov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company