public inbox for [email protected]
help / color / mirror / Atom feedFrom: Greg Burd <[email protected]>
To: David Rowley <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: [PATCH] Add tests for Bitmapset
Date: Tue, 30 Sep 2025 08:38:48 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAApHDvq-4994jpDnspQ+bgSbzo0+=Z4YQbj+YfP7wi97B0e1qg@mail.gmail.com>
References: <CAApHDvq-4994jpDnspQ+bgSbzo0+=Z4YQbj+YfP7wi97B0e1qg@mail.gmail.com>
On Sep 30 2025, at 6:29 am, David Rowley <[email protected]> wrote:
> On Tue, 30 Sept 2025 at 22:30, Greg Burd <[email protected]> wrote:
>> Thank you both, patch attached.
>
> Patch looks good to me.
>
> Just while reviewing, I was confused at the following code in
> test_bms_add_range().
>
> /* Check for invalid range */
> if (upper < lower)
> {
> bms_free(bms);
> PG_RETURN_NULL();
> }
>
> That seems wrong. You should just return the input set in that case,
> not NULL.
Agreed, I should have caught this inconsistency earlier on.
> This results in:
>
> postgres=# select test_bms_add_range('(b 1)', 2, 5); -- ok.
> test_bms_add_range
> --------------------
> (b 1 2 3 4 5)
> (1 row)
>
> postgres=# select test_bms_add_range('(b 1)', 5, 2); -- wrong results
> test_bms_add_range
> --------------------
>
> (1 row)
>
> I'd expect the last one to return '(b 1)', effectively the input set unmodified.
>
> If I remove the code quoted above, I also see something else unexpected:
>
> postgres=# select test_bms_add_range('(b)', 5, 2);
> test_bms_add_range
> --------------------
> <>
> (1 row)
>
> Now, you might blame me for that as I see you've done things like:
>
> if (bms_is_empty(bms))
> PG_RETURN_NULL();
>
> in other places, but it's not very consistent as I can get the <> in
> other locations:
>
> postgres=# select test_bms_add_members('(b)', '(b)');
> test_bms_add_members
> ----------------------
> <>
> (1 row)
>
> It seems to me that you could save some code and all this
> inconsistency by just making <> the standard way to represent an empty
> Bitmapset. Or if you really want to keep the SQL NULLs, you could make
> a helper macro that handles that consistently.
nodeToString() will turn NULL into '<>' because it has no way to know
that we're referring to a NULL/empty Bitmapset, but that's not what we'd
ideally like to return in those cases because NULL Bitmapsets are in
fact a thing. The standard/consistent representation for a NULL
Bitmapset encoded by nodeToString() is '(b)' so I'll update to be more
consistent across tests.
> For me, I think stripping as much logic out of the test functions as
> possible is a good way of doing things. I expect you really want to
> witness the behaviour of bitmapset.c, not some anomaly of
> test_bitmapset.c.
You are 100% correct, I've stripped away as much as possible so as to
push the responsibility for results down to the bitmapset.c code as it
should be.
> David
v2 attached.
best.
-greg
Attachments:
[application/octet-stream] v2-0001-Remove-NULL-check-and-other-defensive-code-in-Bit.patch (20.3K, 2-v2-0001-Remove-NULL-check-and-other-defensive-code-in-Bit.patch)
download | inline diff:
From 8199c3e0205a7eb956061be86b472a86ed266ad8 Mon Sep 17 00:00:00 2001
From: Greg Burd <[email protected]>
Date: Tue, 30 Sep 2025 05:15:40 -0400
Subject: [PATCH v2] Remove NULL check and other defensive code in Bitmapset
tests
Some tests overshadowed their callee's responsiblity to check arguments
and be defensive or return default values, this is unnecessary and can
hide changes in the underlying code. In addition, the bms_free()
function safely handles NULL input, so checking for NULL before calling
it is unnecessary and again hides an important aspect of the API. Since
tests serve as reference examples for proper API usage, they should
demonstrate the correct calling patterns and not duplicate error
checking in the functions they expose for testing purposes.
Author: Greg Burd <[email protected]>
Reported-by: David Rowley<[email protected]>
Reviewed-by: Michael Paquier <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
.../expected/test_bitmapset.out | 78 ++---
.../modules/test_bitmapset/test_bitmapset.c | 285 ++++++------------
2 files changed, 139 insertions(+), 224 deletions(-)
diff --git a/src/test/modules/test_bitmapset/expected/test_bitmapset.out b/src/test/modules/test_bitmapset/expected/test_bitmapset.out
index be7b6399c82..2e204d687df 100644
--- a/src/test/modules/test_bitmapset/expected/test_bitmapset.out
+++ b/src/test/modules/test_bitmapset/expected/test_bitmapset.out
@@ -63,7 +63,7 @@ SELECT test_bms_add_member('(b 10)', 10) AS result;
SELECT test_bms_add_member('(b)', NULL) AS result;
result
--------
-
+ (b)
(1 row)
-- bms_replace_members()
@@ -76,7 +76,7 @@ SELECT test_bms_replace_members(NULL, '(b 1 2 3)') AS result;
SELECT test_bms_replace_members('(b 1 2 3)', NULL) AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_replace_members('(b 1 2 3)', '(b 3 5 6)') AS result;
@@ -108,13 +108,13 @@ SELECT test_bms_replace_members('(b 1 2 3 4 5)', '(b 500 600)') AS result;
SELECT test_bms_replace_members('(b 1 2 3)', NULL) AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_replace_members('(b 5)', NULL) AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_replace_members(NULL, '(b 5)') AS result;
@@ -126,7 +126,7 @@ SELECT test_bms_replace_members(NULL, '(b 5)') AS result;
SELECT test_bms_replace_members(NULL, NULL) AS result;
result
--------
-
+ (b)
(1 row)
-- bms_del_member()
@@ -135,13 +135,13 @@ ERROR: negative bitmapset member not allowed
SELECT test_bms_del_member('(b)', 10) AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_del_member('(b 10)', 10) AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_del_member('(b 10)', 5) AS result;
@@ -193,26 +193,26 @@ SELECT test_bms_del_member('(b 1 50 100 200)', 100) AS result;
SELECT test_bms_del_member('(b 42)', 42) AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_del_member('(b 5)', NULL) AS result;
result
--------
-
+ (b 5)
(1 row)
-- bms_del_members()
SELECT test_bms_del_members('(b)', '(b 10)') AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_del_members('(b 10)', '(b 10)') AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_del_members('(b 10)', '(b 5)') AS result;
@@ -262,7 +262,7 @@ SELECT test_bms_del_members('(b 5)', NULL) AS result;
SELECT test_bms_del_members(NULL, '(b 5)') AS result;
result
--------
-
+ (b)
(1 row)
-- bms_join()
@@ -319,7 +319,7 @@ SELECT test_bms_join(NULL, '(b 5)') AS result;
SELECT test_bms_join(NULL, NULL) AS result;
result
--------
-
+ (b)
(1 row)
-- bms_union()
@@ -341,7 +341,7 @@ SELECT test_bms_union('(b 1 3 5)', '(b)') AS result;
SELECT test_bms_union('(b)', '(b)') AS result;
result
--------
-
+ (b)
(1 row)
-- Overlapping ranges
@@ -383,7 +383,7 @@ SELECT test_bms_union(NULL, '(b 5)') AS result;
SELECT test_bms_union(NULL, NULL) AS result;
result
--------
-
+ (b)
(1 row)
-- bms_intersect()
@@ -398,14 +398,14 @@ SELECT test_bms_intersect('(b 1 3 5)', '(b 3 5 7)') AS result;
SELECT test_bms_intersect('(b 1 3 5)', '(b 2 4 6)') AS result;
result
--------
-
+ (b)
(1 row)
-- Intersect with empty
SELECT test_bms_intersect('(b 1 3 5)', '(b)') AS result;
result
--------
-
+ (b)
(1 row)
-- Intersect with varrying word counts
@@ -425,25 +425,25 @@ SELECT test_bms_intersect('(b 1 2 3 4 5)', '(b 1 300)') AS result;
SELECT test_bms_intersect('(b 1)', '(b 2)') AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_intersect('(b 5)', NULL) AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_intersect(NULL, '(b 5)') AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_intersect(NULL, NULL) AS result;
result
--------
-
+ (b)
(1 row)
-- bms_int_members()
@@ -458,14 +458,14 @@ SELECT test_bms_int_members('(b 1 3 5)', '(b 3 5 7)') AS result;
SELECT test_bms_int_members('(b 1 3 5)', '(b 2 4 6)') AS result;
result
--------
-
+ (b)
(1 row)
-- Intersect with empty
SELECT test_bms_int_members('(b 1 3 5)', '(b)') AS result;
result
--------
-
+ (b)
(1 row)
-- Multiple members
@@ -479,25 +479,25 @@ SELECT test_bms_int_members('(b 0 31 32 63 64)', '(b 31 32 64 65)') AS result;
SELECT test_bms_int_members('(b 1)', '(b 2)') AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_int_members('(b 5)', NULL) AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_int_members(NULL, '(b 5)') AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_int_members(NULL, NULL) AS result;
result
--------
-
+ (b)
(1 row)
-- bms_difference()
@@ -519,14 +519,14 @@ SELECT test_bms_difference('(b 1 3 5)', '(b 2 4 6)') AS result;
SELECT test_bms_difference('(b 1 3 5)', '(b 1 3 5)') AS result;
result
--------
-
+ (b)
(1 row)
-- Substraction to empty
SELECT test_bms_difference('(b 42)', '(b 42)') AS result;
result
--------
-
+ (b)
(1 row)
-- Subtraction edge case
@@ -556,7 +556,7 @@ SELECT test_bms_difference('(b 1 2 100 200)', '(b 1 2)') AS result;
SELECT test_bms_difference('(b 5)', '(b 5 10)') AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_difference('(b 5)', NULL) AS result;
@@ -568,13 +568,13 @@ SELECT test_bms_difference('(b 5)', NULL) AS result;
SELECT test_bms_difference(NULL, '(b 5)') AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_difference(NULL, NULL) AS result;
result
--------
-
+ (b)
(1 row)
-- bms_is_member()
@@ -887,19 +887,19 @@ SELECT length(test_bms_add_range('(b 1 2)', 200, 250)) AS result;
SELECT test_bms_add_range('(b 5)', 5, NULL) AS result;
result
--------
-
+ (b 5)
(1 row)
SELECT test_bms_add_range('(b 5)', NULL, 10) AS result;
result
--------
-
+ (b 5)
(1 row)
SELECT test_bms_add_range('(b 5)', NULL, NULL) AS result;
result
--------
-
+ (b 5)
(1 row)
SELECT test_bms_add_range(NULL, 5, 10) AS result;
@@ -911,13 +911,13 @@ SELECT test_bms_add_range(NULL, 5, 10) AS result;
SELECT test_bms_add_range(NULL, 10, 5) AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_add_range(NULL, NULL, NULL) AS result;
result
--------
-
+ (b)
(1 row)
-- bms_membership()
@@ -1421,7 +1421,7 @@ SELECT test_bms_subset_compare(NULL, NULL) AS result;
SELECT test_bms_copy(NULL) AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_copy('(b 1 3 5 7)') AS result;
@@ -1434,7 +1434,7 @@ SELECT test_bms_copy('(b 1 3 5 7)') AS result;
SELECT test_bms_copy(NULL) AS result;
result
--------
-
+ (b)
(1 row)
-- bms_add_members()
diff --git a/src/test/modules/test_bitmapset/test_bitmapset.c b/src/test/modules/test_bitmapset/test_bitmapset.c
index 2e821320836..93d83105c2d 100644
--- a/src/test/modules/test_bitmapset/test_bitmapset.c
+++ b/src/test/modules/test_bitmapset/test_bitmapset.c
@@ -26,7 +26,6 @@
#include "nodes/pg_list.h"
#include "utils/builtins.h"
#include "utils/timestamp.h"
-#include "varatt.h"
PG_MODULE_MAGIC;
@@ -84,8 +83,10 @@ PG_FUNCTION_INFO_V1(test_random_operations);
#expr, __FILE__, __LINE__); \
} while (0)
+static const char *empty = "(b)";
+
/* Encode/Decode to/from TEXT and Bitmapset */
-#define BITMAPSET_TO_TEXT(bms) cstring_to_text(nodeToString(bms))
+#define BITMAPSET_TO_TEXT(bms) cstring_to_text(bms ? nodeToString(bms) : empty)
#define TEXT_TO_BITMAPSET(str) ((Bitmapset *) stringToNode(text_to_cstring(str)))
/*
@@ -100,7 +101,7 @@ test_bms_add_member(PG_FUNCTION_ARGS)
text *result;
if (PG_ARGISNULL(1))
- PG_RETURN_NULL();
+ PG_RETURN_TEXT_P(PG_GETARG_TEXT_PP(0));
if (!PG_ARGISNULL(0))
bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
@@ -109,8 +110,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 +131,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);
@@ -148,7 +147,7 @@ test_bms_del_member(PG_FUNCTION_ARGS)
text *result;
if (PG_ARGISNULL(1))
- PG_RETURN_NULL();
+ PG_RETURN_TEXT_P(PG_GETARG_TEXT_PP(0));
if (!PG_ARGISNULL(0))
bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
@@ -156,9 +155,6 @@ test_bms_del_member(PG_FUNCTION_ARGS)
member = PG_GETARG_INT32(1);
bms = bms_del_member(bms, member);
- if (bms_is_empty(bms))
- PG_RETURN_NULL();
-
result = BITMAPSET_TO_TEXT(bms);
bms_free(bms);
@@ -181,8 +177,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 +193,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);
}
@@ -212,7 +206,7 @@ test_bms_make_singleton(PG_FUNCTION_ARGS)
text *result;
if (PG_ARGISNULL(0))
- PG_RETURN_NULL();
+ PG_RETURN_TEXT_P(BITMAPSET_TO_TEXT(NULL));
member = PG_GETARG_INT32(0);
bms = bms_make_singleton(member);
@@ -226,23 +220,18 @@ test_bms_make_singleton(PG_FUNCTION_ARGS)
Datum
test_bms_copy(PG_FUNCTION_ARGS)
{
- text *bms_data;
Bitmapset *bms = NULL;
Bitmapset *copy_bms;
text *result;
- if (PG_ARGISNULL(0))
- PG_RETURN_NULL();
+ if (!PG_ARGISNULL(0))
+ bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
- bms_data = PG_GETARG_TEXT_PP(0);
- bms = TEXT_TO_BITMAPSET(bms_data);
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 +251,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,13 +273,8 @@ test_bms_union(PG_FUNCTION_ARGS)
result_bms = bms_union(bms1, bms2);
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
-
- if (result_bms == NULL)
- PG_RETURN_NULL();
+ bms_free(bms1);
+ bms_free(bms2);
result = BITMAPSET_TO_TEXT(result_bms);
bms_free(result_bms);
@@ -306,14 +288,12 @@ test_bms_membership(PG_FUNCTION_ARGS)
Bitmapset *bms = NULL;
BMS_Membership result;
- if (PG_ARGISNULL(0))
- PG_RETURN_INT32(BMS_EMPTY_SET);
+ if (!PG_ARGISNULL(0))
+ bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
- 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);
}
@@ -325,15 +305,17 @@ test_bms_next_member(PG_FUNCTION_ARGS)
int32 prevmember;
int result;
- if (PG_ARGISNULL(0) || PG_ARGISNULL(1))
+ if (PG_ARGISNULL(1))
PG_RETURN_INT32(-2);
- bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
prevmember = PG_GETARG_INT32(1);
+
+ if (!PG_ARGISNULL(0))
+ bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
+
result = bms_next_member(bms, prevmember);
- if (bms)
- bms_free(bms);
+ bms_free(bms);
PG_RETURN_INT32(result);
}
@@ -354,13 +336,8 @@ test_bms_intersect(PG_FUNCTION_ARGS)
result_bms = bms_intersect(bms1, bms2);
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
-
- if (result_bms == NULL)
- PG_RETURN_NULL();
+ bms_free(bms1);
+ bms_free(bms2);
result = BITMAPSET_TO_TEXT(result_bms);
bms_free(result_bms);
@@ -384,13 +361,8 @@ test_bms_difference(PG_FUNCTION_ARGS)
result_bms = bms_difference(bms1, bms2);
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
-
- if (result_bms == NULL)
- PG_RETURN_NULL();
+ bms_free(bms1);
+ bms_free(bms2);
result = BITMAPSET_TO_TEXT(result_bms);
bms_free(result_bms);
@@ -413,10 +385,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 +402,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 +422,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 +443,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 +460,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);
}
@@ -505,46 +469,38 @@ Datum
test_bms_get_singleton_member(PG_FUNCTION_ARGS)
{
Bitmapset *bms = NULL;
- int32 default_member = PG_GETARG_INT32(1);
- bool success;
int member = -1;
- if (PG_ARGISNULL(0))
- PG_RETURN_INT32(default_member);
+ if (!PG_ARGISNULL(1))
+ member = PG_GETARG_INT32(1);
- bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
+ if (!PG_ARGISNULL(0))
+ bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
/*
* bms_get_singleton_member returns bool and stores result in member
- * pointer
+ * pointer, we don't change the value of member when supplied so we ignore
+ * the return value.
*/
- success = bms_get_singleton_member(bms, &member);
+ (void) bms_get_singleton_member(bms, &member);
bms_free(bms);
- if (success)
- PG_RETURN_INT32(member);
-
- PG_RETURN_INT32(default_member);
+ PG_RETURN_INT32(member);
}
Datum
test_bms_prev_member(PG_FUNCTION_ARGS)
{
- text *bms_data;
Bitmapset *bms = NULL;
- int32 prevmember;
+ int32 prevmember = 0;
int result;
- if (PG_ARGISNULL(0))
- PG_RETURN_INT32(-2);
-
- bms_data = PG_GETARG_TEXT_PP(0);
- prevmember = PG_GETARG_INT32(1);
+ if (!PG_ARGISNULL(0))
+ bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
- if (VARSIZE_ANY_EXHDR(bms_data) == 0)
- PG_RETURN_INT32(-2);
+ if (!PG_ARGISNULL(1))
+ prevmember = PG_GETARG_INT32(1);
- bms = TEXT_TO_BITMAPSET(bms_data);
result = bms_prev_member(bms, prevmember);
bms_free(bms);
@@ -566,10 +522,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);
}
@@ -581,48 +535,44 @@ test_bms_overlap_list(PG_FUNCTION_ARGS)
ArrayType *array;
List *int_list = NIL;
bool result;
- Datum *elem_datums;
- bool *elem_nulls;
+ Datum *elem_datums = NULL;
+ bool *elem_nulls = NULL;
int elem_count;
int i;
- if (PG_ARGISNULL(0))
- PG_RETURN_BOOL(false);
-
- bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
+ if (!PG_ARGISNULL(0))
+ bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
- if (PG_ARGISNULL(1))
+ if (!PG_ARGISNULL(1))
{
- if (bms)
- bms_free(bms);
- PG_RETURN_BOOL(false);
- }
-
- array = PG_GETARG_ARRAYTYPE_P(1);
+ array = PG_GETARG_ARRAYTYPE_P(1);
- deconstruct_array(array,
- INT4OID, sizeof(int32), true, 'i',
- &elem_datums, &elem_nulls, &elem_count);
+ deconstruct_array(array,
+ INT4OID, sizeof(int32), true, 'i',
+ &elem_datums, &elem_nulls, &elem_count);
- for (i = 0; i < elem_count; i++)
- {
- if (!elem_nulls[i])
+ for (i = 0; i < elem_count; i++)
{
- int32 member = DatumGetInt32(elem_datums[i]);
+ if (!elem_nulls[i])
+ {
+ int32 member = DatumGetInt32(elem_datums[i]);
- int_list = lappend_int(int_list, member);
+ int_list = lappend_int(int_list, member);
+ }
}
}
result = bms_overlap_list(bms, int_list);
- if (bms)
- bms_free(bms);
+ bms_free(bms);
list_free(int_list);
- pfree(elem_datums);
- pfree(elem_nulls);
+ if (elem_datums)
+ pfree(elem_datums);
+
+ if (elem_nulls)
+ pfree(elem_nulls);
PG_RETURN_BOOL(result);
}
@@ -642,10 +592,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);
}
@@ -653,21 +601,15 @@ test_bms_nonempty_difference(PG_FUNCTION_ARGS)
Datum
test_bms_member_index(PG_FUNCTION_ARGS)
{
- text *bms_data;
Bitmapset *bms = NULL;
- int32 member;
+ int32 member = -1;
int result;
- if (PG_ARGISNULL(0))
- PG_RETURN_INT32(-1);
-
- bms_data = PG_GETARG_TEXT_PP(0);
- member = PG_GETARG_INT32(1);
-
- if (VARSIZE_ANY_EXHDR(bms_data) == 0)
- PG_RETURN_INT32(-1);
+ if (!PG_ARGISNULL(0))
+ bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
- bms = TEXT_TO_BITMAPSET(bms_data);
+ if (!PG_ARGISNULL(1))
+ member = PG_GETARG_INT32(1);
result = bms_member_index(bms, member);
bms_free(bms);
@@ -683,28 +625,22 @@ test_bms_add_range(PG_FUNCTION_ARGS)
upper;
text *result;
- if (PG_ARGISNULL(1) || PG_ARGISNULL(2))
- PG_RETURN_NULL();
+ lower = upper = 0;
- if (!PG_ARGISNULL(0))
- bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
+ if (!PG_ARGISNULL(1))
+ lower = PG_GETARG_INT32(1);
- lower = PG_GETARG_INT32(1);
- upper = PG_GETARG_INT32(2);
+ if (!PG_ARGISNULL(2))
+ upper = PG_GETARG_INT32(2);
- /* Check for invalid range */
- if (upper < lower)
- {
- if (bms)
- bms_free(bms);
- PG_RETURN_NULL();
- }
+ if (!PG_ARGISNULL(0))
+ bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
- bms = bms_add_range(bms, lower, upper);
+ if (!(PG_ARGISNULL(1) || PG_ARGISNULL(2)))
+ 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 +660,11 @@ test_bms_int_members(PG_FUNCTION_ARGS)
bms1 = bms_int_members(bms1, bms2);
- if (bms2)
- bms_free(bms2);
-
- if (bms1 == NULL)
- PG_RETURN_NULL();
+ bms_free(bms2);
result = BITMAPSET_TO_TEXT(bms1);
- if (bms1)
- bms_free(bms1);
+ bms_free(bms1);
PG_RETURN_TEXT_P(result);
}
@@ -757,11 +688,7 @@ test_bms_del_members(PG_FUNCTION_ARGS)
/* bms1 is now invalid, do not free it */
- if (bms2)
- bms_free(bms2);
-
- if (result_bms == NULL)
- PG_RETURN_NULL();
+ bms_free(bms2);
result = BITMAPSET_TO_TEXT(result_bms);
bms_free(result_bms);
@@ -788,11 +715,7 @@ test_bms_replace_members(PG_FUNCTION_ARGS)
/* bms1 is now invalid, do not free it */
- if (bms2)
- bms_free(bms2);
-
- if (result_bms == NULL)
- PG_RETURN_NULL();
+ bms_free(bms2);
result = BITMAPSET_TO_TEXT(result_bms);
bms_free(result_bms);
@@ -819,9 +742,6 @@ test_bms_join(PG_FUNCTION_ARGS)
/* bms1 and bms2 may have been recycled! Do not free any of them. */
- if (result_bms == NULL)
- PG_RETURN_NULL();
-
result = BITMAPSET_TO_TEXT(result_bms);
bms_free(result_bms);
@@ -839,8 +759,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 +808,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 +947,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