Re: Why assignment before return?

This code-pattern appears many times in pgstatfuncs.c:

Datum
pg_stat_get_blocks_fetched(PG_FUNCTION_ARGS)
{
Oid relid = PG_GETARG_OID(0);
int64 result;
PgStat_StatTabEntry *tabentry;

if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
result = 0;
else
result = (int64) (tabentry->blocks_fetched);

PG_RETURN_INT64(result);
}

Why do we assign this to "result" and then return, why not just:
if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
PG_RETURN_INT64(0);
else
PG_RETURN_INT64(tabentry->blocks_fetched);

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On 20 August 2010 12:46, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> This code-pattern appears many times in pgstatfuncs.c:
>
> Datum
> pg_stat_get_blocks_fetched(PG_FUNCTION_ARGS)
> {
>        Oid                     relid = PG_GETARG_OID(0);
>        int64           result;
>        PgStat_StatTabEntry *tabentry;
>
>        if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
>                result = 0;
>        else
>                result = (int64) (tabentry->blocks_fetched);
>
>        PG_RETURN_INT64(result);
> }
>
>
> Why do we assign this to "result" and then return, why not just:
>        if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
>                PG_RETURN_INT64(0);
>        else
>                PG_RETURN_INT64(tabentry->blocks_fetched);
>
>
> --

And then drop the "int64 result;" declaration as a result.

--
Thom Brown
Registered Linux user: #516935

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> This code-pattern appears many times in pgstatfuncs.c:
> Datum
> pg_stat_get_blocks_fetched(PG_FUNCTION_ARGS)
> {
> Oid relid = PG_GETARG_OID(0);
> int64 result;
> PgStat_StatTabEntry *tabentry;

> if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
> result = 0;
> else
> result = (int64) (tabentry->blocks_fetched);

> PG_RETURN_INT64(result);
> }

I see nothing wrong with that style. Reducing it as you propose
probably wouldn't change the emitted code at all, and what it would
do is reduce flexibility. For instance, if we ever needed to add
additional operations just before the RETURN (releasing a lock on
the tabentry, perhaps) we'd just have to undo the "improvement".

regards, tom lane

On Fri, Aug 20, 2010 at 15:10, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> This code-pattern appears many times in pgstatfuncs.c:
>> Datum
>> pg_stat_get_blocks_fetched(PG_FUNCTION_ARGS)
>> {
>>       Oid                     relid = PG_GETARG_OID(0);
>>       int64           result;
>>       PgStat_StatTabEntry *tabentry;
>
>>       if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
>>               result = 0;
>>       else
>>               result = (int64) (tabentry->blocks_fetched);
>
>>       PG_RETURN_INT64(result);
>> }
>
>
> I see nothing wrong with that style.  Reducing it as you propose
> probably wouldn't change the emitted code at all, and what it would
> do is reduce flexibility.  For instance, if we ever needed to add
> additional operations just before the RETURN (releasing a lock on
> the tabentry, perhaps) we'd just have to undo the "improvement".

I'm not saying it's wrong, I'm just trying to figure out why it's
there since I wanted to add other functions and it looked.. Odd. I'll
change my new functions to look like this for consistency, but I was
curious if there was some specific reason why it was better to do it
this way.

I see your answer as "no, not really any reason, but also not worth
changing", which is fine by me :-)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> I see your answer as "no, not really any reason, but also not worth
> changing", which is fine by me :-)

Yeah, that's a fair summary. If it had been coded the other way
to start with, I'd also say it wasn't worth changing, at least
not until such time as we actually needed to.

In the meantime, any added functions of the same ilk should definitely
be made to look like the existing ones.

regards, tom lane

On Fri, Aug 20, 2010 at 15:27, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> I see your answer as "no, not really any reason, but also not worth
>> changing", which is fine by me :-)
>
> Yeah, that's a fair summary.  If it had been coded the other way
> to start with, I'd also say it wasn't worth changing, at least
> not until such time as we actually needed to.
>
> In the meantime, any added functions of the same ilk should definitely
> be made to look like the existing ones.

Yeah. I notice there are some functions that are not following this
pattern, but most are, so I'll adjust my patch with this.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/