On 04/03/15 23:51, Andreas Karlsson wrote:
> On 01/29/2015 12:28 AM, Peter Geoghegan wrote:
>> * I'm not sure about the idea of "polymorphic" catalog functions (that
>> return the type "internal", but the actual struct returned varying
>> based on build settings).
>>
>> I tend to think that things would be better if there was always a
>> uniform return type for such "internal" type returning functions, but
>> that *its* structure varied according to the availability of int128
>> (i.e. HAVE_INT128) at compile time. What we should probably do is have
>> a third aggregate struct, that encapsulates this idea of (say)
>> int2_accum() piggybacking on one of either Int128AggState or
>> NumericAggState directly. Maybe that would be called PolyNumAggState.
>> Then this common code is all that is needed on both types of build (at
>> the top of int4_accum(), for example):
>>
>> PolyNumAggState *state;
>>
>> state = PG_ARGISNULL(0) ? NULL : (PolyNumAggState *)
>> PG_GETARG_POINTER(0);
>>
>> I'm not sure if I'd do this with a containing struct or a simple
>> "#ifdef HAVE_INT128 typedef #else ... ", but I think it's an
>> improvement either way.
>
> Not entirely sure exactly what I mean but I have changed to something
> like this, relying on typedef, in the attached version of the patch. I
> think it looks better after the changes.
>
I think this made the patch much better indeed.
What I am wondering is if those numeric_int16_* functions that also deal
with either the Int128AggState or NumericAggState should be renamed in
similar fashion.
-- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services