public inbox for [email protected]
help / color / mirror / Atom feedFrom: Greg Burd <[email protected]>
To: Michael Paquier <[email protected]>
To: David Rowley <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: [PATCH] Add tests for Bitmapset
Date: Tue, 30 Sep 2025 05:30:51 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
On Sep 29 2025, at 8:51 pm, Michael Paquier <[email protected]> wrote:
> On Tue, Sep 30, 2025 at 01:19:05PM +1300, David Rowley wrote:
>> NULL is a valid Bitmapset, so I don't really see the need to check for
>> an empty set before calling bms_free(). If those were removed, then
>> you'd not have to care about the coverage of that line.
Good point, I agree. Thanks for reporting/reviewing David.
> Yeah, we could just remove them as you are suggesting, as well.
> --
> Michael
Thank you both, patch attached.
best.
-greg
Attachments:
[application/octet-stream] v1-0001-Don-t-check-for-NULL-before-calling-bms_free-in-t.patch (7.1K, 2-v1-0001-Don-t-check-for-NULL-before-calling-bms_free-in-t.patch)
download | inline diff:
From 8c8fc8c7dc1e0c7752c35a33044f107540f94e61 Mon Sep 17 00:00:00 2001
From: Greg Burd <[email protected]>
Date: Tue, 30 Sep 2025 05:15:40 -0400
Subject: [PATCH v1] Don't check for NULL before calling bms_free() in tests
The bms_free() function safely handles NULL input, so checking for NULL
before calling it is unnecessary. Since tests serve as reference examples
for proper API usage, they should demonstrate the correct calling pattern
without redundant NULL checks.
Author: Greg Burd <[email protected]>
Reported-by: David Rowley<[email protected]>
Reviewed-by: Michael Paquier <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
.../modules/test_bitmapset/test_bitmapset.c | 121 ++++++------------
1 file changed, 40 insertions(+), 81 deletions(-)
diff --git a/src/test/modules/test_bitmapset/test_bitmapset.c b/src/test/modules/test_bitmapset/test_bitmapset.c
index 2e821320836..7a706662e9f 100644
--- a/src/test/modules/test_bitmapset/test_bitmapset.c
+++ b/src/test/modules/test_bitmapset/test_bitmapset.c
@@ -109,8 +109,7 @@ test_bms_add_member(PG_FUNCTION_ARGS)
bms = bms_add_member(bms, member);
result = BITMAPSET_TO_TEXT(bms);
- if (bms)
- bms_free(bms);
+ bms_free(bms);
PG_RETURN_TEXT_P(result);
}
@@ -131,8 +130,7 @@ test_bms_add_members(PG_FUNCTION_ARGS)
/* IMPORTANT: bms_add_members modifies/frees the first argument */
bms1 = bms_add_members(bms1, bms2);
- if (bms2)
- bms_free(bms2);
+ bms_free(bms2);
result = BITMAPSET_TO_TEXT(bms1);
bms_free(bms1);
@@ -181,8 +179,7 @@ test_bms_is_member(PG_FUNCTION_ARGS)
member = PG_GETARG_INT32(1);
result = bms_is_member(member, bms);
- if (bms)
- bms_free(bms);
+ bms_free(bms);
PG_RETURN_BOOL(result);
}
@@ -198,8 +195,7 @@ test_bms_num_members(PG_FUNCTION_ARGS)
result = bms_num_members(bms);
- if (bms)
- bms_free(bms);
+ bms_free(bms);
PG_RETURN_INT32(result);
}
@@ -239,10 +235,8 @@ test_bms_copy(PG_FUNCTION_ARGS)
copy_bms = bms_copy(bms);
result = BITMAPSET_TO_TEXT(copy_bms);
- if (bms)
- bms_free(bms);
- if (copy_bms)
- bms_free(copy_bms);
+ bms_free(bms);
+ bms_free(copy_bms);
PG_RETURN_TEXT_P(result);
}
@@ -262,10 +256,8 @@ test_bms_equal(PG_FUNCTION_ARGS)
result = bms_equal(bms1, bms2);
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
+ bms_free(bms1);
+ bms_free(bms2);
PG_RETURN_BOOL(result);
}
@@ -286,10 +278,8 @@ test_bms_union(PG_FUNCTION_ARGS)
result_bms = bms_union(bms1, bms2);
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
+ bms_free(bms1);
+ bms_free(bms2);
if (result_bms == NULL)
PG_RETURN_NULL();
@@ -312,8 +302,7 @@ test_bms_membership(PG_FUNCTION_ARGS)
bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
result = bms_membership(bms);
- if (bms)
- bms_free(bms);
+ bms_free(bms);
PG_RETURN_INT32((int32) result);
}
@@ -332,8 +321,7 @@ test_bms_next_member(PG_FUNCTION_ARGS)
prevmember = PG_GETARG_INT32(1);
result = bms_next_member(bms, prevmember);
- if (bms)
- bms_free(bms);
+ bms_free(bms);
PG_RETURN_INT32(result);
}
@@ -354,10 +342,8 @@ test_bms_intersect(PG_FUNCTION_ARGS)
result_bms = bms_intersect(bms1, bms2);
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
+ bms_free(bms1);
+ bms_free(bms2);
if (result_bms == NULL)
PG_RETURN_NULL();
@@ -384,10 +370,8 @@ test_bms_difference(PG_FUNCTION_ARGS)
result_bms = bms_difference(bms1, bms2);
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
+ bms_free(bms1);
+ bms_free(bms2);
if (result_bms == NULL)
PG_RETURN_NULL();
@@ -413,10 +397,8 @@ test_bms_compare(PG_FUNCTION_ARGS)
result = bms_compare(bms1, bms2);
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
+ bms_free(bms1);
+ bms_free(bms2);
PG_RETURN_INT32(result);
}
@@ -432,8 +414,7 @@ test_bms_is_empty(PG_FUNCTION_ARGS)
result = bms_is_empty(bms);
- if (bms)
- bms_free(bms);
+ bms_free(bms);
PG_RETURN_BOOL(result);
}
@@ -453,10 +434,8 @@ test_bms_is_subset(PG_FUNCTION_ARGS)
result = bms_is_subset(bms1, bms2);
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
+ bms_free(bms1);
+ bms_free(bms2);
PG_RETURN_BOOL(result);
}
@@ -476,10 +455,8 @@ test_bms_subset_compare(PG_FUNCTION_ARGS)
result = bms_subset_compare(bms1, bms2);
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
+ bms_free(bms1);
+ bms_free(bms2);
PG_RETURN_INT32((int32) result);
}
@@ -495,8 +472,7 @@ test_bms_singleton_member(PG_FUNCTION_ARGS)
result = bms_singleton_member(bms);
- if (bms)
- bms_free(bms);
+ bms_free(bms);
PG_RETURN_INT32(result);
}
@@ -566,10 +542,8 @@ test_bms_overlap(PG_FUNCTION_ARGS)
result = bms_overlap(bms1, bms2);
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
+ bms_free(bms1);
+ bms_free(bms2);
PG_RETURN_BOOL(result);
}
@@ -593,8 +567,7 @@ test_bms_overlap_list(PG_FUNCTION_ARGS)
if (PG_ARGISNULL(1))
{
- if (bms)
- bms_free(bms);
+ bms_free(bms);
PG_RETURN_BOOL(false);
}
@@ -616,8 +589,7 @@ test_bms_overlap_list(PG_FUNCTION_ARGS)
result = bms_overlap_list(bms, int_list);
- if (bms)
- bms_free(bms);
+ bms_free(bms);
list_free(int_list);
@@ -642,10 +614,8 @@ test_bms_nonempty_difference(PG_FUNCTION_ARGS)
result = bms_nonempty_difference(bms1, bms2);
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
+ bms_free(bms1);
+ bms_free(bms2);
PG_RETURN_BOOL(result);
}
@@ -695,16 +665,14 @@ test_bms_add_range(PG_FUNCTION_ARGS)
/* Check for invalid range */
if (upper < lower)
{
- if (bms)
- bms_free(bms);
+ bms_free(bms);
PG_RETURN_NULL();
}
bms = bms_add_range(bms, lower, upper);
result = BITMAPSET_TO_TEXT(bms);
- if (bms)
- bms_free(bms);
+ bms_free(bms);
PG_RETURN_TEXT_P(result);
}
@@ -724,16 +692,14 @@ test_bms_int_members(PG_FUNCTION_ARGS)
bms1 = bms_int_members(bms1, bms2);
- if (bms2)
- bms_free(bms2);
+ bms_free(bms2);
if (bms1 == NULL)
PG_RETURN_NULL();
result = BITMAPSET_TO_TEXT(bms1);
- if (bms1)
- bms_free(bms1);
+ bms_free(bms1);
PG_RETURN_TEXT_P(result);
}
@@ -757,8 +723,7 @@ test_bms_del_members(PG_FUNCTION_ARGS)
/* bms1 is now invalid, do not free it */
- if (bms2)
- bms_free(bms2);
+ bms_free(bms2);
if (result_bms == NULL)
PG_RETURN_NULL();
@@ -788,8 +753,7 @@ test_bms_replace_members(PG_FUNCTION_ARGS)
/* bms1 is now invalid, do not free it */
- if (bms2)
- bms_free(bms2);
+ bms_free(bms2);
if (result_bms == NULL)
PG_RETURN_NULL();
@@ -839,8 +803,7 @@ test_bms_hash_value(PG_FUNCTION_ARGS)
hash_result = bms_hash_value(bms);
- if (bms)
- bms_free(bms);
+ bms_free(bms);
PG_RETURN_INT32(hash_result);
}
@@ -889,11 +852,8 @@ test_bitmap_match(PG_FUNCTION_ARGS)
/* Call bitmap_match with addresses of the Bitmapset pointers */
match_result = bitmap_match(&bms_ptr1, &bms_ptr2, sizeof(Bitmapset *));
- /* Clean up */
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
+ bms_free(bms1);
+ bms_free(bms2);
PG_RETURN_INT32(match_result);
}
@@ -1031,8 +991,7 @@ test_random_operations(PG_FUNCTION_ARGS)
total_ops++;
}
- if (bms)
- bms_free(bms);
+ bms_free(bms);
PG_RETURN_INT32(total_ops);
}
--
2.49.0
view thread (81+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Add tests for Bitmapset
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox