On 10/18/2017 11:47 AM, Dmitry Dolgov wrote:
> > On 16 October 2017 at 02:08, Michael Paquier
> <michael.paquier@gmail.com <mailto:michael.paquier@gmail.com>> wrote:
> >
> >> Thanks for the patch. Everything looks fine, I just have one question -
> >> maybe it makes sense to put this pattern `if (variadic) {/*...*/}`
> into a
> >> separate function? Because from what I see it's basically one part
> of code
> >> (I didn't spot any difference) repeated four times for every function.
> >
> > The json/jsonb calls have one difference though. For jsonb, arguments
> > with unknown type are enforced to text, which is not the case of json,
> > and we don't want to change that behavior.
>
> Oh, actually what I meant is to put into a separate function only the
> first
> branch of this condition, i.e. when `variadic` is true. But in general
> I don't
> really have strong opinion about that, so if you think that this
> repetition is
> not a problem, then fine.
>
> > There is a common place for JSON-common code in jsonapi.c, but
> jsonapi.h is
> > wanted as light-weight (see its set of headers), so moving things
> there is
> > incorrect as well IMO.
>
> Hm...did you mean `jsonfuncs.c` and `jsonapi.h`? It seems to me, that this
> potential function (to handle `variadic` case) would not really
> pollute it.
If we really wanted to we could have it as a parameter to the function
to do the special UNKONOWNOID handling or not.
But I'm not sure it's worth it, especially on the back branches where we
should try to do minimal code disturbance.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
