Thread: Are we missing (void) when return value of fsm_set_and_search is ignored?

Are we missing (void) when return value of fsm_set_and_search is ignored?

From
Bharath Rupireddy
Date:
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



Re: Are we missing (void) when return value of fsm_set_and_search is ignored?

From
Bharath Rupireddy
Date:
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.



Re: Are we missing (void) when return value of fsm_set_and_search is ignored?

From
Julien Rouhaud
Date:
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



Re: Are we missing (void) when return value of fsm_set_and_search is ignored?

From
Bharath Rupireddy
Date:
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.

Attachment

Re: Are we missing (void) when return value of fsm_set_and_search is ignored?

From
Peter Eisentraut
Date:
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.



Re: Are we missing (void) when return value of fsm_set_and_search is ignored?

From
Julien Rouhaud
Date:
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.



Re: Are we missing (void) when return value of fsm_set_and_search is ignored?

From
Bharath Rupireddy
Date:
On Fri, Jun 4, 2021 at 9:58 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > 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.

Yeah, just for consistency reasons (void) casting can be added to fsm_set_and_search when it's return value is ignored.

With Regards,
Bharath Rupireddy.

Re: Are we missing (void) when return value of fsm_set_and_search is ignored?

From
Peter Eisentraut
Date:
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.



Re: Are we missing (void) when return value of fsm_set_and_search is ignored?

From
Bharath Rupireddy
Date:
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.



Re: Are we missing (void) when return value of fsm_set_and_search is ignored?

From
Julien Rouhaud
Date:
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.