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.
Attachments:
v1-0001-Use-void-when-return-value-of-fsm_set_and_search-.patchapplication/octet-stream; name=v1-0001-Use-void-when-return-value-of-fsm_set_and_search-.patchDownload
From b4625db1d0757adc5b96e69dea2c2332f495c079 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Thu, 3 Jun 2021 05:52:43 -0700
Subject: [PATCH v1] Use (void) when return value of fsm_set_and_search is
ignored
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) casting. In the code base, we generally use (void)
when non-void return value of a function is ignored.
Although it is not a bug or not giving any compiler warning, just
use (void) as it seems to be a standard practice in the code base.
---
src/backend/storage/freespace/freespace.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/storage/freespace/freespace.c b/src/backend/storage/freespace/freespace.c
index 8c12dda238..14e7540230 100644
--- a/src/backend/storage/freespace/freespace.c
+++ b/src/backend/storage/freespace/freespace.c
@@ -187,7 +187,7 @@ RecordPageWithFreeSpace(Relation rel, BlockNumber heapBlk, Size spaceAvail)
/* Get the location of the FSM byte representing the heap block */
addr = fsm_get_location(heapBlk, &slot);
- fsm_set_and_search(rel, addr, slot, new_cat, 0);
+ (void) fsm_set_and_search(rel, addr, slot, new_cat, 0);
}
/*
@@ -752,7 +752,7 @@ fsm_search(Relation rel, uint8 min_cat)
* rarely, and will be fixed by the next vacuum.
*/
parent = fsm_get_parent(addr, &parentslot);
- fsm_set_and_search(rel, parent, parentslot, max_avail, 0);
+ (void) fsm_set_and_search(rel, parent, parentslot, max_avail, 0);
/*
* If the upper pages are badly out of date, we might need to loop
--
2.25.1
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 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.
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.