Re: Transform for pl/perl
От | Anthony Bykov |
---|---|
Тема | Re: Transform for pl/perl |
Дата | |
Msg-id | 20171207125602.5326f103@anthony-24-g082ur обсуждение исходный текст |
Ответ на | Re: Transform for pl/perl (Anthony Bykov <a.bykov@postgrespro.ru>) |
Ответы |
Re: Transform for pl/perl
(Thomas Munro <thomas.munro@enterprisedb.com>)
|
Список | pgsql-hackers |
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
Вложения
В списке pgsql-hackers по дате отправления: