Re: Transform for pl/perl

On Fri, 1 Dec 2017 15:49:21 -0300
Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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