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
