Обсуждение: Are we missing (void) when return value of fsm_set_and_search is ignored?

Hi,

It looks like for some of the fsm_set_and_search calls whose return
value is ignored (in fsm_search and RecordPageWithFreeSpace), there's
no (void). Is it intentional? In the code base, we generally have
(void) when non-void return value of a function is ignored.

Thoughts?

With Regards,
Bharath Rupireddy.

On Thu, Jun 3, 2021 at 4:24 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Hi,
>
> It looks like for some of the fsm_set_and_search calls whose return
> value is ignored (in fsm_search and RecordPageWithFreeSpace), there's
> no (void). Is it intentional?

Basically, fsm_set_and_search, serve both "set" and "search", but it
only search if the "minValue" is > 0.  So if the minvalue is passed as
0 then the return value is ignored intentionally.  I can see in both
places where the returned value is ignored the minvalue is passed as
0.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Thu, Jun 3, 2021 at 4:47 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, Jun 3, 2021 at 4:24 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > Hi,
> >
> > It looks like for some of the fsm_set_and_search calls whose return
> > value is ignored (in fsm_search and RecordPageWithFreeSpace), there's
> > no (void). Is it intentional?
>
> Basically, fsm_set_and_search, serve both "set" and "search", but it
> only search if the "minValue" is > 0.  So if the minvalue is passed as
> 0 then the return value is ignored intentionally.  I can see in both
> places where the returned value is ignored the minvalue is passed as
> 0.

Thanks. I know why we are ignoring the return value. I was trying to
say, when we ignore (for whatsoever reason it maybe) return value of
any non-void returning function, we do something like below right?

(void) fsm_set_and_search(rel, addr, slot, new_cat, 0);

instead of

fsm_set_and_search(rel, addr, slot, new_cat, 0);

With Regards,
Bharath Rupireddy.

On Thu, Jun 3, 2021 at 6:54 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> It looks like for some of the fsm_set_and_search calls whose return
> value is ignored (in fsm_search and RecordPageWithFreeSpace), there's
> no (void). Is it intentional? In the code base, we generally have
> (void) when non-void return value of a function is ignored.

That's a good practice, +1 for changing that.

On Thu, Jun 3, 2021 at 5:11 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Thu, Jun 3, 2021 at 4:47 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Thu, Jun 3, 2021 at 4:24 PM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > It looks like for some of the fsm_set_and_search calls whose return
> > > value is ignored (in fsm_search and RecordPageWithFreeSpace), there's
> > > no (void). Is it intentional?
> >
> > Basically, fsm_set_and_search, serve both "set" and "search", but it
> > only search if the "minValue" is > 0.  So if the minvalue is passed as
> > 0 then the return value is ignored intentionally.  I can see in both
> > places where the returned value is ignored the minvalue is passed as
> > 0.
>
> Thanks. I know why we are ignoring the return value. I was trying to
> say, when we ignore (for whatsoever reason it maybe) return value of
> any non-void returning function, we do something like below right?
>
> (void) fsm_set_and_search(rel, addr, slot, new_cat, 0);
>
> instead of
>
> fsm_set_and_search(rel, addr, slot, new_cat, 0);

Okay, I thought you were asking whether we are ignoring the return
value is intentional or not.  Yeah, typecasting the return with void
is a better practice for ignoring the return value.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Thu, Jun 3, 2021 at 5:22 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Thu, Jun 3, 2021 at 6:54 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > It looks like for some of the fsm_set_and_search calls whose return
> > value is ignored (in fsm_search and RecordPageWithFreeSpace), there's
> > no (void). Is it intentional? In the code base, we generally have
> > (void) when non-void return value of a function is ignored.
>
> That's a good practice, +1 for changing that.

Thanks. PSA v1 patch.

With Regards,
Bharath Rupireddy.

On 03.06.21 12:54, Bharath Rupireddy wrote:
> It looks like for some of the fsm_set_and_search calls whose return
> value is ignored (in fsm_search and RecordPageWithFreeSpace), there's
> no (void). Is it intentional? In the code base, we generally have
> (void) when non-void return value of a function is ignored.

I don't think that is correct.  I don't see anyone writing

(void) printf(...);

so this is not a generally applicable strategy.

We have pg_nodiscard for functions where you explicitly want callers to 
check the return value.  In all other cases, callers are free to ignore 
return values.

On Thu, Jun 03, 2021 at 02:57:42PM +0200, Peter Eisentraut wrote:
> On 03.06.21 12:54, Bharath Rupireddy wrote:
> > It looks like for some of the fsm_set_and_search calls whose return
> > value is ignored (in fsm_search and RecordPageWithFreeSpace), there's
> > no (void). Is it intentional? In the code base, we generally have
> > (void) when non-void return value of a function is ignored.
> 
> I don't think that is correct.  I don't see anyone writing
> 
> (void) printf(...);

We somehow do have a (void) fprint(...) in src/port/getopt.c.

> so this is not a generally applicable strategy.
> 
> We have pg_nodiscard for functions where you explicitly want callers to
> check the return value.  In all other cases, callers are free to ignore
> return values.

Yes, but we have a lot a examples of functions without pg_nodiscard and callers
still explicitly ignoring the results, like fsm_vacuum_page() in the same file.
It would be more consistent and make the code slightly more self explanatory.

On 04.06.21 06:28, Julien Rouhaud wrote:
> Yes, but we have a lot a examples of functions without pg_nodiscard and callers
> still explicitly ignoring the results, like fsm_vacuum_page() in the same file.
> It would be more consistent and make the code slightly more self explanatory.

I'm not clear how you'd make a guideline out of this, other than, "it's 
also done elsewhere".

In this case I'd actually split the function in two, one that returns 
void and one that always returns a value to be consumed.  This 
overloading is a bit confusing.

On Sat, Jun 5, 2021 at 1:38 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> On 04.06.21 06:28, Julien Rouhaud wrote:
> > Yes, but we have a lot a examples of functions without pg_nodiscard and callers
> > still explicitly ignoring the results, like fsm_vacuum_page() in the same file.
> > It would be more consistent and make the code slightly more self explanatory.
>
> I'm not clear how you'd make a guideline out of this, other than, "it's
> also done elsewhere".

I proposed to do (void) fsm_set_and_search by looking at lot of other
places (more than few 100) in the code base like (void)
defGetBoolean(def) (void) hv_iterinit(obj) (void) set_config_option(
and so on. I'm not sure whether having consistent code in a few
hundred places amounts to a standard practice.

> In this case I'd actually split the function in two, one that returns
> void and one that always returns a value to be consumed.  This
> overloading is a bit confusing.

Thanks. I don't want to go in that direction. Instead I choose to
withdraw the proposal here and let the fsm_set_and_search function
usage be as is.

With Regards,
Bharath Rupireddy.

On Sat, Jun 5, 2021 at 4:08 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> On 04.06.21 06:28, Julien Rouhaud wrote:
> > Yes, but we have a lot a examples of functions without pg_nodiscard and callers
> > still explicitly ignoring the results, like fsm_vacuum_page() in the same file.
> > It would be more consistent and make the code slightly more self explanatory.
>
> I'm not clear how you'd make a guideline out of this, other than, "it's
> also done elsewhere".

When it can be confusing, like here?

> In this case I'd actually split the function in two, one that returns
> void and one that always returns a value to be consumed.  This
> overloading is a bit confusing.

That would work too, but it may be overkill as it's not a public API.