On Fri, Jul 29, 2022 at 10:55 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, Jul 28, 2022 at 10:29 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > > I have done some cleanup in 0002 as well, basically, earlier we were
> > > storing the result of the BufTagGetRelFileLocator() in a separate
> > > variable which is not required everywhere. So wherever possible I
> > > have avoided using the intermediate variable.
> >
> > I'll have a look at this next.
>
> I was taught that when programming in C one should avoid returning a
> struct type, as BufTagGetRelFileLocator does. I would have expected it
> to return void and take an argument of type RelFileLocator * into
> which it writes the results. On the other hand, I was also taught that
> one should avoid passing a struct type as an argument, and smgropen()
> has been doing that since Tom Lane committed
> 87bd95638552b8fc1f5f787ce5b862bb6fc2eb80 all the way back in 2004. So
> maybe this isn't that relevant any more on modern compilers? Or maybe
> for small structs it doesn't matter much? I dunno.
>
> Other than that, I think your 0002 looks fine.
Generally, I try to avoid it, but I see in current code also if the
structure is small and by directly returning the structure it makes
the other code easy then we are doing this way[1]. I wanted to do
this way is a) if we pass as an argument then I will have to use an
extra variable which makes some code complicated, it's not a big
issue, infact I had it that way in the previous version but simplified
in one of the recent versions. b) If I allocate memory and return
pointer then also I need to store that address and later free that.
[1]
static inline ForEachState
for_each_from_setup(const List *lst, int N)
{
ForEachState r = {lst, N};
Assert(N >= 0);
return r;
}
static inline FullTransactionId
FullTransactionIdFromEpochAndXid(uint32 epoch, TransactionId xid)
{
FullTransactionId result;
result.value = ((uint64) epoch) << 32 | xid;
return result;
}
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com