Why assignment before return?

On 20 August 2010 12:46, Magnus Hagander <magnus@hagander.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@hagander.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@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.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@hagander.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@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.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/