test_bitmapset: Simplify code of the module

Two macros are added in this module, to cut duplicated patterns:
- PG_ARG_GETBITMAPSET(), for input argument handling, with knowledge
about NULL.
- PG_RETURN_BITMAPSET_AS_TEXT(), that generates a text result from a
Bitmapset.

These changes limit the code so as the SQL functions are now mostly
wrappers of the equivalent C function.  Functions that use integer input
arguments still need some NULL handling, like bms_make_singleton().

A NULL input is translated to "<>", which is what nodeToString()
generates.  Some of the tests are able to generate this result.

Per discussion, the calls of bms_free() are removed.  These may be
justified if the functions are used in a rather long-lived memory
context, but let's keep the code minimal for now.  These calls used NULL
checks, which were also not necessary as NULL is an input authorized by
bms_free().

Some of the tests existed to cover behaviors related to the SQL
functions for NULL inputs.  Most of them are still relevant, as the
routines of bitmapset.c are able to handle such cases.

The coverage reports of bitmapset.c and test_bitmapset.c remain the
same after these changes, with 300 lines of C code removed.

Author: David Rowley <dgrowleyml@gmail.com>
Co-authored-by: Greg Burd <greg@burd.me>
Discussion: https://postgr.es/m/CAApHDvqghMnm_zgSNefto9oaEJ0S-3Cgb3gdsV7XvLC-hMS02Q@mail.gmail.com

1 files changed, 152 insertions, 463 deletions

diff --git a/src/test/modules/test_bitmapset/test_bitmapset.c b/src/test/modules/test_bitmapset/test_bitmapset.c
index 2e821320836..6cf8a257187 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;
 
@@ -89,100 +88,102 @@ PG_FUNCTION_INFO_V1(test_random_operations);
 #define TEXT_TO_BITMAPSET(str) ((Bitmapset *) stringToNode(text_to_cstring(str)))
 
 /*
+ * Helper macro to fetch text parameters as Bitmapsets. SQL-NULL means empty
+ * set.
+ */
+#define PG_ARG_GETBITMAPSET(n) \
+	(PG_ARGISNULL(n) ? NULL : TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(n)))
+
+/*
+ * Helper macro to handle converting sets back to text, freeing the set and
+ * returning the resulting text representation of the set.  Beware of double
+ * evaluation hazard of 'bms'.
+ */
+#define PG_RETURN_BITMAPSET_AS_TEXT(bms) \
+	do { \
+		text	   *result = BITMAPSET_TO_TEXT(bms); \
+		PG_RETURN_TEXT_P(result); \
+	} while (0);
+
+/*
  * Individual test functions for each bitmapset API function
+ *
+ * Primarily, we aim to keep these as close to simple wrapper functions as
+ * possible in order to publish the functions of bitmapset.c to the SQL layer
+ * with as little interference as possible.  We opt to return SQL NULL in
+ * cases where the input given to the SQL function isn't valid to pass to the
+ * underlying bitmapset.c function.  For example we cannot do much useful
+ * testing if someone calls test_bms_make_singleton(NULL) since
+ * bms_make_singleton() expects an integer argument.
+ *
+ * For function arguments which are to be converted to Bitmapsets, we accept
+ * SQL NULL as a valid argument to mean an empty set.  Optionally callers may
+ * pass '(b)'.
+ *
+ * For the test functions which return a Bitmapset, these are converted back
+ * to text with result generated by nodeToString().
  */
 
 Datum
 test_bms_add_member(PG_FUNCTION_ARGS)
 {
+	Bitmapset  *bms;
 	int			member;
-	Bitmapset  *bms = NULL;
-	text	   *result;
 
 	if (PG_ARGISNULL(1))
-		PG_RETURN_NULL();
-
-	if (!PG_ARGISNULL(0))
-		bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
+		PG_RETURN_NULL();		/* invalid input */
 
+	bms = PG_ARG_GETBITMAPSET(0);
 	member = PG_GETARG_INT32(1);
-	bms = bms_add_member(bms, member);
-	result = BITMAPSET_TO_TEXT(bms);
 
-	if (bms)
-		bms_free(bms);
+	bms = bms_add_member(bms, member);
 
-	PG_RETURN_TEXT_P(result);
+	PG_RETURN_BITMAPSET_AS_TEXT(bms);
 }
 
 Datum
 test_bms_add_members(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms1 = NULL,
-			   *bms2 = NULL;
-	text	   *result;
-
-	if (!PG_ARGISNULL(0))
-		bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
-	if (!PG_ARGISNULL(1))
-		bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
+	Bitmapset  *bms1 = PG_ARG_GETBITMAPSET(0);
+	Bitmapset  *bms2 = PG_ARG_GETBITMAPSET(1);
 
-	/* IMPORTANT: bms_add_members modifies/frees the first argument */
+	/* left input is recycled */
 	bms1 = bms_add_members(bms1, bms2);
 
-	if (bms2)
-		bms_free(bms2);
-
-	result = BITMAPSET_TO_TEXT(bms1);
-	bms_free(bms1);
-
-	PG_RETURN_TEXT_P(result);
+	PG_RETURN_BITMAPSET_AS_TEXT(bms1);
 }
 
 Datum
 test_bms_del_member(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms = NULL;
+	Bitmapset  *bms;
 	int32		member;
-	text	   *result;
 
 	if (PG_ARGISNULL(1))
-		PG_RETURN_NULL();
-
-	if (!PG_ARGISNULL(0))
-		bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
+		PG_RETURN_NULL();		/* invalid input */
 
+	bms = PG_ARG_GETBITMAPSET(0);
 	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);
+	bms = bms_del_member(bms, member);
 
-	PG_RETURN_TEXT_P(result);
+	PG_RETURN_BITMAPSET_AS_TEXT(bms);
 }
 
 Datum
 test_bms_is_member(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms = NULL;
+	Bitmapset  *bms;
 	int32		member;
 	bool		result;
 
 	if (PG_ARGISNULL(1))
-		PG_RETURN_BOOL(false);
-
-	if (!PG_ARGISNULL(0))
-		bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
+		PG_RETURN_NULL();		/* invalid input */
 
+	bms = PG_ARG_GETBITMAPSET(0);
 	member = PG_GETARG_INT32(1);
-	result = bms_is_member(member, bms);
 
-	if (bms)
-		bms_free(bms);
+	result = bms_is_member(member, bms);
 
 	PG_RETURN_BOOL(result);
 }
@@ -190,150 +191,89 @@ test_bms_is_member(PG_FUNCTION_ARGS)
 Datum
 test_bms_num_members(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms = NULL;
-	int			result = 0;
-
-	if (!PG_ARGISNULL(0))
-		bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
+	Bitmapset  *bms = PG_ARG_GETBITMAPSET(0);
+	int			result;
 
 	result = bms_num_members(bms);
 
-	if (bms)
-		bms_free(bms);
-
 	PG_RETURN_INT32(result);
 }
 
 Datum
 test_bms_make_singleton(PG_FUNCTION_ARGS)
 {
-	int32		member;
 	Bitmapset  *bms;
-	text	   *result;
+	int32		member;
 
 	if (PG_ARGISNULL(0))
-		PG_RETURN_NULL();
+		PG_RETURN_NULL();		/* invalid input */
 
 	member = PG_GETARG_INT32(0);
 	bms = bms_make_singleton(member);
 
-	result = BITMAPSET_TO_TEXT(bms);
-	bms_free(bms);
-
-	PG_RETURN_TEXT_P(result);
+	PG_RETURN_BITMAPSET_AS_TEXT(bms);
 }
 
 Datum
 test_bms_copy(PG_FUNCTION_ARGS)
 {
-	text	   *bms_data;
-	Bitmapset  *bms = NULL;
+	Bitmapset  *bms = PG_ARG_GETBITMAPSET(0);
 	Bitmapset  *copy_bms;
-	text	   *result;
-
-	if (PG_ARGISNULL(0))
-		PG_RETURN_NULL();
 
-	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);
-
-	PG_RETURN_TEXT_P(result);
+	PG_RETURN_BITMAPSET_AS_TEXT(copy_bms);
 }
 
 Datum
 test_bms_equal(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms1 = NULL,
-			   *bms2 = NULL;
+	Bitmapset  *bms1 = PG_ARG_GETBITMAPSET(0);
+	Bitmapset  *bms2 = PG_ARG_GETBITMAPSET(1);
 	bool		result;
 
-	if (!PG_ARGISNULL(0))
-		bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
-	if (!PG_ARGISNULL(1))
-		bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
-
 	result = bms_equal(bms1, bms2);
 
-	if (bms1)
-		bms_free(bms1);
-	if (bms2)
-		bms_free(bms2);
-
 	PG_RETURN_BOOL(result);
 }
 
 Datum
 test_bms_union(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms1 = NULL,
-			   *bms2 = NULL;
+	Bitmapset  *bms1 = PG_ARG_GETBITMAPSET(0);
+	Bitmapset  *bms2 = PG_ARG_GETBITMAPSET(1);
 	Bitmapset  *result_bms;
-	text	   *result;
-
-	if (!PG_ARGISNULL(0))
-		bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
-	if (!PG_ARGISNULL(1))
-		bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
 
 	result_bms = bms_union(bms1, bms2);
 
-	if (bms1)
-		bms_free(bms1);
-	if (bms2)
-		bms_free(bms2);
-
-	if (result_bms == NULL)
-		PG_RETURN_NULL();
-
-	result = BITMAPSET_TO_TEXT(result_bms);
-	bms_free(result_bms);
-
-	PG_RETURN_TEXT_P(result);
+	PG_RETURN_BITMAPSET_AS_TEXT(result_bms);
 }
 
 Datum
 test_bms_membership(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms = NULL;
+	Bitmapset  *bms = PG_ARG_GETBITMAPSET(0);
 	BMS_Membership result;
 
-	if (PG_ARGISNULL(0))
-		PG_RETURN_INT32(BMS_EMPTY_SET);
-
-	bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
 	result = bms_membership(bms);
 
-	if (bms)
-		bms_free(bms);
-
 	PG_RETURN_INT32((int32) result);
 }
 
 Datum
 test_bms_next_member(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms = NULL;
+	Bitmapset  *bms;
 	int32		prevmember;
 	int			result;
 
-	if (PG_ARGISNULL(0) || PG_ARGISNULL(1))
-		PG_RETURN_INT32(-2);
+	if (PG_ARGISNULL(1))
+		PG_RETURN_NULL();		/* invalid input */
 
-	bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
+	bms = PG_ARG_GETBITMAPSET(0);
 	prevmember = PG_GETARG_INT32(1);
-	result = bms_next_member(bms, prevmember);
 
-	if (bms)
-		bms_free(bms);
+	result = bms_next_member(bms, prevmember);
 
 	PG_RETURN_INT32(result);
 }
@@ -341,212 +281,115 @@ test_bms_next_member(PG_FUNCTION_ARGS)
 Datum
 test_bms_intersect(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms1 = NULL,
-			   *bms2 = NULL;
+	Bitmapset  *bms1 = PG_ARG_GETBITMAPSET(0);
+	Bitmapset  *bms2 = PG_ARG_GETBITMAPSET(1);
 	Bitmapset  *result_bms;
-	text	   *result;
-
-	if (!PG_ARGISNULL(0))
-		bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
-	if (!PG_ARGISNULL(1))
-		bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
 
 	result_bms = bms_intersect(bms1, bms2);
 
-	if (bms1)
-		bms_free(bms1);
-	if (bms2)
-		bms_free(bms2);
-
-	if (result_bms == NULL)
-		PG_RETURN_NULL();
-
-	result = BITMAPSET_TO_TEXT(result_bms);
-	bms_free(result_bms);
-
-	PG_RETURN_TEXT_P(result);
+	PG_RETURN_BITMAPSET_AS_TEXT(result_bms);
 }
 
 Datum
 test_bms_difference(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms1 = NULL,
-			   *bms2 = NULL;
+	Bitmapset  *bms1 = PG_ARG_GETBITMAPSET(0);
+	Bitmapset  *bms2 = PG_ARG_GETBITMAPSET(1);
 	Bitmapset  *result_bms;
-	text	   *result;
-
-	if (!PG_ARGISNULL(0))
-		bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
-	if (!PG_ARGISNULL(1))
-		bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
 
 	result_bms = bms_difference(bms1, bms2);
 
-	if (bms1)
-		bms_free(bms1);
-	if (bms2)
-		bms_free(bms2);
-
-	if (result_bms == NULL)
-		PG_RETURN_NULL();
-
-	result = BITMAPSET_TO_TEXT(result_bms);
-	bms_free(result_bms);
-
-	PG_RETURN_TEXT_P(result);
+	PG_RETURN_BITMAPSET_AS_TEXT(result_bms);
 }
 
 Datum
 test_bms_compare(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms1 = NULL,
-			   *bms2 = NULL;
+	Bitmapset  *bms1 = PG_ARG_GETBITMAPSET(0);
+	Bitmapset  *bms2 = PG_ARG_GETBITMAPSET(1);
 	int			result;
 
-	if (!PG_ARGISNULL(0))
-		bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
-	if (!PG_ARGISNULL(1))
-		bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
-
 	result = bms_compare(bms1, bms2);
 
-	if (bms1)
-		bms_free(bms1);
-	if (bms2)
-		bms_free(bms2);
-
 	PG_RETURN_INT32(result);
 }
 
 Datum
 test_bms_is_empty(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms = NULL;
+	Bitmapset  *bms = PG_ARG_GETBITMAPSET(0);
 	bool		result;
 
-	if (!PG_ARGISNULL(0))
-		bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
 	result = bms_is_empty(bms);
 
-	if (bms)
-		bms_free(bms);
-
 	PG_RETURN_BOOL(result);
 }
 
 Datum
 test_bms_is_subset(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms1 = NULL,
-			   *bms2 = NULL;
+	Bitmapset  *bms1 = PG_ARG_GETBITMAPSET(0);
+	Bitmapset  *bms2 = PG_ARG_GETBITMAPSET(1);
 	bool		result;
 
-	if (!PG_ARGISNULL(0))
-		bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
-	if (!PG_ARGISNULL(1))
-		bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
-
 	result = bms_is_subset(bms1, bms2);
 
-	if (bms1)
-		bms_free(bms1);
-	if (bms2)
-		bms_free(bms2);
-
 	PG_RETURN_BOOL(result);
 }
 
 Datum
 test_bms_subset_compare(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms1 = NULL,
-			   *bms2 = NULL;
+	Bitmapset  *bms1 = PG_ARG_GETBITMAPSET(0);
+	Bitmapset  *bms2 = PG_ARG_GETBITMAPSET(1);
 	BMS_Comparison result;
 
-	if (!PG_ARGISNULL(0))
-		bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
-	if (!PG_ARGISNULL(1))
-		bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
-
 	result = bms_subset_compare(bms1, bms2);
 
-	if (bms1)
-		bms_free(bms1);
-	if (bms2)
-		bms_free(bms2);
-
 	PG_RETURN_INT32((int32) result);
 }
 
 Datum
 test_bms_singleton_member(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms = NULL;
+	Bitmapset  *bms = PG_ARG_GETBITMAPSET(0);
 	int			result;
 
-	if (!PG_ARGISNULL(0))
-		bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
 	result = bms_singleton_member(bms);
 
-	if (bms)
-		bms_free(bms);
-
 	PG_RETURN_INT32(result);
 }
 
 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);
-
-	bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
+	Bitmapset  *bms = PG_ARG_GETBITMAPSET(0);
+	int			member;
 
 	/*
-	 * bms_get_singleton_member returns bool and stores result in member
-	 * pointer
+	 * Keep this simple.  Return -1 when we detect the set is not a singleton
+	 * set, otherwise return the singleton member.
 	 */
-	success = bms_get_singleton_member(bms, &member);
-	bms_free(bms);
-
-	if (success)
-		PG_RETURN_INT32(member);
+	if (!bms_get_singleton_member(bms, &member))
+		member = -1;
 
-	PG_RETURN_INT32(default_member);
+	PG_RETURN_INT32(member);
 }
 
 Datum
 test_bms_prev_member(PG_FUNCTION_ARGS)
 {
-	text	   *bms_data;
-	Bitmapset  *bms = NULL;
+	Bitmapset  *bms;
 	int32		prevmember;
 	int			result;
 
-	if (PG_ARGISNULL(0))
-		PG_RETURN_INT32(-2);
+	if (PG_ARGISNULL(1))
+		PG_RETURN_NULL();		/* invalid input */
 
-	bms_data = PG_GETARG_TEXT_PP(0);
+	bms = PG_ARG_GETBITMAPSET(0);
 	prevmember = PG_GETARG_INT32(1);
 
-	if (VARSIZE_ANY_EXHDR(bms_data) == 0)
-		PG_RETURN_INT32(-2);
-
-	bms = TEXT_TO_BITMAPSET(bms_data);
 	result = bms_prev_member(bms, prevmember);
-	bms_free(bms);
 
 	PG_RETURN_INT32(result);
 }
@@ -554,75 +397,57 @@ test_bms_prev_member(PG_FUNCTION_ARGS)
 Datum
 test_bms_overlap(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms1 = NULL,
-			   *bms2 = NULL;
+	Bitmapset  *bms1 = PG_ARG_GETBITMAPSET(0);
+	Bitmapset  *bms2 = PG_ARG_GETBITMAPSET(1);
 	bool		result;
 
-	if (!PG_ARGISNULL(0))
-		bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
-	if (!PG_ARGISNULL(1))
-		bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
-
 	result = bms_overlap(bms1, bms2);
 
-	if (bms1)
-		bms_free(bms1);
-	if (bms2)
-		bms_free(bms2);
-
 	PG_RETURN_BOOL(result);
 }
 
 Datum
 test_bms_overlap_list(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms = NULL;
+	Bitmapset  *bms;
 	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));
+	bms = PG_ARG_GETBITMAPSET(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);
-
 	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);
 }
@@ -630,47 +455,29 @@ test_bms_overlap_list(PG_FUNCTION_ARGS)
 Datum
 test_bms_nonempty_difference(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms1 = NULL,
-			   *bms2 = NULL;
+	Bitmapset  *bms1 = PG_ARG_GETBITMAPSET(0);
+	Bitmapset  *bms2 = PG_ARG_GETBITMAPSET(1);
 	bool		result;
 
-	if (!PG_ARGISNULL(0))
-		bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
-	if (!PG_ARGISNULL(1))
-		bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
-
 	result = bms_nonempty_difference(bms1, bms2);
 
-	if (bms1)
-		bms_free(bms1);
-	if (bms2)
-		bms_free(bms2);
-
 	PG_RETURN_BOOL(result);
 }
 
 Datum
 test_bms_member_index(PG_FUNCTION_ARGS)
 {
-	text	   *bms_data;
-	Bitmapset  *bms = NULL;
+	Bitmapset  *bms;
 	int32		member;
 	int			result;
 
-	if (PG_ARGISNULL(0))
-		PG_RETURN_INT32(-1);
+	if (PG_ARGISNULL(1))
+		PG_RETURN_NULL();		/* invalid input */
 
-	bms_data = PG_GETARG_TEXT_PP(0);
+	bms = PG_ARG_GETBITMAPSET(0);
 	member = PG_GETARG_INT32(1);
 
-	if (VARSIZE_ANY_EXHDR(bms_data) == 0)
-		PG_RETURN_INT32(-1);
-
-	bms = TEXT_TO_BITMAPSET(bms_data);
-
 	result = bms_member_index(bms, member);
-	bms_free(bms);
 
 	PG_RETURN_INT32(result);
 }
@@ -678,191 +485,92 @@ test_bms_member_index(PG_FUNCTION_ARGS)
 Datum
 test_bms_add_range(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms = NULL;
+	Bitmapset  *bms;
 	int32		lower,
 				upper;
-	text	   *result;
 
 	if (PG_ARGISNULL(1) || PG_ARGISNULL(2))
-		PG_RETURN_NULL();
-
-	if (!PG_ARGISNULL(0))
-		bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
+		PG_RETURN_NULL();		/* invalid input */
 
+	bms = PG_ARG_GETBITMAPSET(0);
 	lower = PG_GETARG_INT32(1);
 	upper = PG_GETARG_INT32(2);
 
-	/* Check for invalid range */
-	if (upper < lower)
-	{
-		if (bms)
-			bms_free(bms);
-		PG_RETURN_NULL();
-	}
-
 	bms = bms_add_range(bms, lower, upper);
 
-	result = BITMAPSET_TO_TEXT(bms);
-	if (bms)
-		bms_free(bms);
-
-	PG_RETURN_TEXT_P(result);
+	PG_RETURN_BITMAPSET_AS_TEXT(bms);
 }
 
 Datum
 test_bms_int_members(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms1 = NULL,
-			   *bms2 = NULL;
-	text	   *result;
-
-	if (!PG_ARGISNULL(0))
-		bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
-	if (!PG_ARGISNULL(1))
-		bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
+	Bitmapset  *bms1 = PG_ARG_GETBITMAPSET(0);
+	Bitmapset  *bms2 = PG_ARG_GETBITMAPSET(1);
 
+	/* left input gets recycled */
 	bms1 = bms_int_members(bms1, bms2);
 
-	if (bms2)
-		bms_free(bms2);
-
-	if (bms1 == NULL)
-		PG_RETURN_NULL();
-
-	result = BITMAPSET_TO_TEXT(bms1);
-
-	if (bms1)
-		bms_free(bms1);
-
-	PG_RETURN_TEXT_P(result);
+	PG_RETURN_BITMAPSET_AS_TEXT(bms1);
 }
 
 Datum
 test_bms_del_members(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms1 = NULL,
-			   *bms2 = NULL;
-	Bitmapset  *result_bms;
-	text	   *result;
+	Bitmapset  *bms1 = PG_ARG_GETBITMAPSET(0);
+	Bitmapset  *bms2 = PG_ARG_GETBITMAPSET(1);
 
-	if (!PG_ARGISNULL(0))
-		bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
+	/* left input gets recycled */
+	bms1 = bms_del_members(bms1, bms2);
 
-	if (!PG_ARGISNULL(1))
-		bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
-
-	/* IMPORTANT: bms_del_members modifies/frees the first argument */
-	result_bms = bms_del_members(bms1, bms2);
-
-	/* bms1 is now invalid, do not free it */
-
-	if (bms2)
-		bms_free(bms2);
-
-	if (result_bms == NULL)
-		PG_RETURN_NULL();
-
-	result = BITMAPSET_TO_TEXT(result_bms);
-	bms_free(result_bms);
-
-	PG_RETURN_TEXT_P(result);
+	PG_RETURN_BITMAPSET_AS_TEXT(bms1);
 }
 
 Datum
 test_bms_replace_members(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms1 = NULL,
-			   *bms2 = NULL;
-	Bitmapset  *result_bms;
-	text	   *result;
-
-	if (!PG_ARGISNULL(0))
-		bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
-	if (!PG_ARGISNULL(1))
-		bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
+	Bitmapset  *bms1 = PG_ARG_GETBITMAPSET(0);
+	Bitmapset  *bms2 = PG_ARG_GETBITMAPSET(1);
 
-	/* IMPORTANT: bms_replace_members modifies/frees the first argument */
-	result_bms = bms_replace_members(bms1, bms2);
+	/* left input gets recycled */
+	bms1 = bms_replace_members(bms1, bms2);
 
-	/* bms1 is now invalid, do not free it */
-
-	if (bms2)
-		bms_free(bms2);
-
-	if (result_bms == NULL)
-		PG_RETURN_NULL();
-
-	result = BITMAPSET_TO_TEXT(result_bms);
-	bms_free(result_bms);
-
-	PG_RETURN_TEXT_P(result);
+	PG_RETURN_BITMAPSET_AS_TEXT(bms1);
 }
 
 Datum
 test_bms_join(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms1 = NULL,
-			   *bms2 = NULL;
+	Bitmapset  *bms1 = PG_ARG_GETBITMAPSET(0);
+	Bitmapset  *bms2 = PG_ARG_GETBITMAPSET(1);
 	Bitmapset  *result_bms;
-	text	   *result;
-
-	if (!PG_ARGISNULL(0))
-		bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
 
-	if (!PG_ARGISNULL(1))
-		bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
-
-	/* IMPORTANT: bms_join may recycle either input arguments */
+	/* either input can be recycled */
 	result_bms = bms_join(bms1, bms2);
 
-	/* 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);
+	/* memory cleanup seems more tricky than it's worth here */
 
-	PG_RETURN_TEXT_P(result);
+	PG_RETURN_BITMAPSET_AS_TEXT(result_bms);
 }
 
 Datum
 test_bms_hash_value(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms = NULL;
+	Bitmapset  *bms = PG_ARG_GETBITMAPSET(0);
 	uint32		hash_result;
 
-	if (!PG_ARGISNULL(0))
-		bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
 	hash_result = bms_hash_value(bms);
 
-	if (bms)
-		bms_free(bms);
-
 	PG_RETURN_INT32(hash_result);
 }
 
 Datum
 test_bitmap_hash(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms = NULL;
-	Bitmapset  *bms_ptr;
+	Bitmapset  *bms = PG_ARG_GETBITMAPSET(0);
 	uint32		hash_result;
 
-	if (!PG_ARGISNULL(0))
-		bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
-	bms_ptr = bms;
-
 	/* Call bitmap_hash */
-	hash_result = bitmap_hash(&bms_ptr, sizeof(Bitmapset *));
-
-	/* Clean up */
-	if (!PG_ARGISNULL(0) && bms_ptr)
-		bms_free(bms_ptr);
+	hash_result = bitmap_hash(&bms, sizeof(Bitmapset *));
 
 	PG_RETURN_INT32(hash_result);
 }
@@ -870,30 +578,12 @@ test_bitmap_hash(PG_FUNCTION_ARGS)
 Datum
 test_bitmap_match(PG_FUNCTION_ARGS)
 {
-	Bitmapset  *bms1 = NULL,
-			   *bms2 = NULL;
-	Bitmapset  *bms_ptr1,
-			   *bms_ptr2;
+	Bitmapset  *bms1 = PG_ARG_GETBITMAPSET(0);
+	Bitmapset  *bms2 = PG_ARG_GETBITMAPSET(1);
 	int			match_result;
 
-	if (!PG_ARGISNULL(0))
-		bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
-	if (!PG_ARGISNULL(1))
-		bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
-
-	/* Set up pointers to the Bitmapsets */
-	bms_ptr1 = bms1;
-	bms_ptr2 = bms2;
-
 	/* 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);
+	match_result = bitmap_match(&bms1, &bms2, sizeof(Bitmapset *));
 
 	PG_RETURN_INT32(match_result);
 }
@@ -1031,8 +721,7 @@ test_random_operations(PG_FUNCTION_ARGS)
 		total_ops++;
 	}
 
-	if (bms)
-		bms_free(bms);
+	bms_free(bms);
 
 	PG_RETURN_INT32(total_ops);
 }