Re: Transform for pl/perl

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