From 7c1832135698abcc0218f6a86e0c1760d4496b8a Mon Sep 17 00:00:00 2001 From: Richard Guo Date: Wed, 27 Dec 2023 15:37:41 +0800 Subject: [PATCH v1] Revise the Asserts added to bimapset manipulation functions The Asserts added to bitmapset.c by commits 71a3e8c43b and 7d58f2342b contain some duplicates, such as in bms_difference, bms_is_subset, bms_subset_compare, bms_int_members and bms_join. Sorry that I failed to notice those duplicates when reviewing the patchset, mainly because they were introduced in different patches. While removing those duplicates, this patch adds checks in the new Asserts to ensure that Bitmapsets should not contain trailing zero words, as the old Asserts did. This patch also defines a macro BITMAPSET_IS_VALID to help check that a Bitmapset is valid. In passing, this patch moves the Asserts to the beginning of functions, just for paranoia's sake. --- src/backend/nodes/bitmapset.c | 108 ++++++++++++++++------------------ 1 file changed, 51 insertions(+), 57 deletions(-) diff --git a/src/backend/nodes/bitmapset.c b/src/backend/nodes/bitmapset.c index e13ecaa155..13ad47ca89 100644 --- a/src/backend/nodes/bitmapset.c +++ b/src/backend/nodes/bitmapset.c @@ -38,6 +38,9 @@ #define BITMAPSET_SIZE(nwords) \ (offsetof(Bitmapset, words) + (nwords) * sizeof(bitmapword)) +#define BITMAPSET_IS_VALID(bms) \ + ((bms) == NULL || (IsA((bms), Bitmapset) && (bms)->words[(bms)->nwords - 1] != 0)) + /*---------- * This is a well-known cute trick for isolating the rightmost one-bit * in a word. It assumes two's complement arithmetic. Consider any @@ -82,9 +85,10 @@ bms_copy(const Bitmapset *a) Bitmapset *result; size_t size; + Assert(BITMAPSET_IS_VALID(a)); + if (a == NULL) return NULL; - Assert(IsA(a, Bitmapset)); size = BITMAPSET_SIZE(a->nwords); result = (Bitmapset *) palloc(size); memcpy(result, a, size); @@ -99,8 +103,8 @@ bms_equal(const Bitmapset *a, const Bitmapset *b) { int i; - Assert(a == NULL || (IsA(a, Bitmapset) && a->words[a->nwords - 1] != 0)); - Assert(b == NULL || (IsA(b, Bitmapset) && b->words[b->nwords - 1] != 0)); + Assert(BITMAPSET_IS_VALID(a)); + Assert(BITMAPSET_IS_VALID(b)); /* Handle cases where either input is NULL */ if (a == NULL) @@ -140,8 +144,8 @@ bms_compare(const Bitmapset *a, const Bitmapset *b) { int i; - Assert(a == NULL || (IsA(a, Bitmapset) && a->words[a->nwords - 1] != 0)); - Assert(b == NULL || (IsA(b, Bitmapset) && b->words[b->nwords - 1] != 0)); + Assert(BITMAPSET_IS_VALID(a)); + Assert(BITMAPSET_IS_VALID(b)); /* Handle cases where either input is NULL */ if (a == NULL) @@ -216,8 +220,8 @@ bms_union(const Bitmapset *a, const Bitmapset *b) int otherlen; int i; - Assert(a == NULL || IsA(a, Bitmapset)); - Assert(b == NULL || IsA(b, Bitmapset)); + Assert(BITMAPSET_IS_VALID(a)); + Assert(BITMAPSET_IS_VALID(b)); /* Handle cases where either input is NULL */ if (a == NULL) @@ -257,8 +261,8 @@ bms_intersect(const Bitmapset *a, const Bitmapset *b) int resultlen; int i; - Assert(a == NULL || IsA(a, Bitmapset)); - Assert(b == NULL || IsA(b, Bitmapset)); + Assert(BITMAPSET_IS_VALID(a)); + Assert(BITMAPSET_IS_VALID(b)); /* Handle cases where either input is NULL */ if (a == NULL || b == NULL) @@ -307,8 +311,8 @@ bms_difference(const Bitmapset *a, const Bitmapset *b) Bitmapset *result; int i; - Assert(a == NULL || (IsA(a, Bitmapset) && a->words[a->nwords - 1] != 0)); - Assert(b == NULL || (IsA(b, Bitmapset) && b->words[b->nwords - 1] != 0)); + Assert(BITMAPSET_IS_VALID(a)); + Assert(BITMAPSET_IS_VALID(b)); /* Handle cases where either input is NULL */ if (a == NULL) @@ -316,8 +320,6 @@ bms_difference(const Bitmapset *a, const Bitmapset *b) if (b == NULL) return bms_copy(a); - Assert(IsA(a, Bitmapset) && IsA(b, Bitmapset)); - /* * In Postgres' usage, an empty result is a very common case, so it's * worth optimizing for that by testing bms_nonempty_difference(). This @@ -374,8 +376,8 @@ bms_is_subset(const Bitmapset *a, const Bitmapset *b) { int i; - Assert(a == NULL || (IsA(a, Bitmapset) && a->words[a->nwords - 1] != 0)); - Assert(b == NULL || (IsA(b, Bitmapset) && b->words[b->nwords - 1] != 0)); + Assert(BITMAPSET_IS_VALID(a)); + Assert(BITMAPSET_IS_VALID(b)); /* Handle cases where either input is NULL */ if (a == NULL) @@ -383,8 +385,6 @@ bms_is_subset(const Bitmapset *a, const Bitmapset *b) if (b == NULL) return false; - Assert(IsA(a, Bitmapset) && IsA(b, Bitmapset)); - /* 'a' can't be a subset of 'b' if it contains more words */ if (a->nwords > b->nwords) return false; @@ -411,8 +411,8 @@ bms_subset_compare(const Bitmapset *a, const Bitmapset *b) int shortlen; int i; - Assert(a == NULL || (IsA(a, Bitmapset) && a->words[a->nwords - 1] != 0)); - Assert(b == NULL || (IsA(b, Bitmapset) && b->words[b->nwords - 1] != 0)); + Assert(BITMAPSET_IS_VALID(a)); + Assert(BITMAPSET_IS_VALID(b)); /* Handle cases where either input is NULL */ if (a == NULL) @@ -424,8 +424,6 @@ bms_subset_compare(const Bitmapset *a, const Bitmapset *b) if (b == NULL) return BMS_SUBSET2; - Assert(IsA(a, Bitmapset) && IsA(b, Bitmapset)); - /* Check common words */ result = BMS_EQUAL; /* status so far */ shortlen = Min(a->nwords, b->nwords); @@ -477,14 +475,13 @@ bms_is_member(int x, const Bitmapset *a) int wordnum, bitnum; + Assert(BITMAPSET_IS_VALID(a)); + /* XXX better to just return false for x<0 ? */ if (x < 0) elog(ERROR, "negative bitmapset member not allowed"); if (a == NULL) return false; - - Assert(IsA(a, Bitmapset)); - wordnum = WORDNUM(x); bitnum = BITNUM(x); if (wordnum >= a->nwords) @@ -509,12 +506,12 @@ bms_member_index(Bitmapset *a, int x) int result = 0; bitmapword mask; + Assert(BITMAPSET_IS_VALID(a)); + /* return -1 if not a member of the bitmap */ if (!bms_is_member(x, a)) return -1; - Assert(IsA(a, Bitmapset)); - wordnum = WORDNUM(x); bitnum = BITNUM(x); @@ -549,8 +546,8 @@ bms_overlap(const Bitmapset *a, const Bitmapset *b) int shortlen; int i; - Assert(a == NULL || IsA(a, Bitmapset)); - Assert(b == NULL || IsA(b, Bitmapset)); + Assert(BITMAPSET_IS_VALID(a)); + Assert(BITMAPSET_IS_VALID(b)); /* Handle cases where either input is NULL */ if (a == NULL || b == NULL) @@ -576,7 +573,7 @@ bms_overlap_list(const Bitmapset *a, const List *b) int wordnum, bitnum; - Assert(a == NULL || IsA(a, Bitmapset)); + Assert(BITMAPSET_IS_VALID(a)); if (a == NULL || b == NIL) return false; @@ -607,8 +604,8 @@ bms_nonempty_difference(const Bitmapset *a, const Bitmapset *b) { int i; - Assert(a == NULL || (IsA(a, Bitmapset) && a->words[a->nwords - 1] != 0)); - Assert(b == NULL || (IsA(b, Bitmapset) && b->words[b->nwords - 1] != 0)); + Assert(BITMAPSET_IS_VALID(a)); + Assert(BITMAPSET_IS_VALID(b)); /* Handle cases where either input is NULL */ if (a == NULL) @@ -640,11 +637,10 @@ bms_singleton_member(const Bitmapset *a) int nwords; int wordnum; + Assert(BITMAPSET_IS_VALID(a)); + if (a == NULL) elog(ERROR, "bitmapset is empty"); - - Assert(IsA(a, Bitmapset)); - nwords = a->nwords; wordnum = 0; do @@ -683,9 +679,10 @@ bms_get_singleton_member(const Bitmapset *a, int *member) int nwords; int wordnum; + Assert(BITMAPSET_IS_VALID(a)); + if (a == NULL) return false; - Assert(IsA(a, Bitmapset)); nwords = a->nwords; wordnum = 0; do @@ -717,9 +714,10 @@ bms_num_members(const Bitmapset *a) int nwords; int wordnum; + Assert(BITMAPSET_IS_VALID(a)); + if (a == NULL) return 0; - Assert(IsA(a, Bitmapset)); nwords = a->nwords; wordnum = 0; do @@ -745,9 +743,10 @@ bms_membership(const Bitmapset *a) int nwords; int wordnum; + Assert(BITMAPSET_IS_VALID(a)); + if (a == NULL) return BMS_EMPTY_SET; - Assert(IsA(a, Bitmapset)); nwords = a->nwords; wordnum = 0; do @@ -786,11 +785,12 @@ bms_add_member(Bitmapset *a, int x) int wordnum, bitnum; + Assert(BITMAPSET_IS_VALID(a)); + if (x < 0) elog(ERROR, "negative bitmapset member not allowed"); if (a == NULL) return bms_make_singleton(x); - Assert(IsA(a, Bitmapset)); wordnum = WORDNUM(x); bitnum = BITNUM(x); @@ -847,13 +847,13 @@ bms_del_member(Bitmapset *a, int x) Bitmapset *tmp = a; #endif + Assert(BITMAPSET_IS_VALID(a)); + if (x < 0) elog(ERROR, "negative bitmapset member not allowed"); if (a == NULL) return NULL; - Assert(IsA(a, Bitmapset)); - wordnum = WORDNUM(x); bitnum = BITNUM(x); @@ -900,8 +900,8 @@ bms_add_members(Bitmapset *a, const Bitmapset *b) int otherlen; int i; - Assert(a == NULL || IsA(a, Bitmapset)); - Assert(b == NULL || IsA(b, Bitmapset)); + Assert(BITMAPSET_IS_VALID(a)); + Assert(BITMAPSET_IS_VALID(b)); /* Handle cases where either input is NULL */ if (a == NULL) @@ -955,7 +955,7 @@ bms_add_range(Bitmapset *a, int lower, int upper) ushiftbits, wordnum; - Assert(a == NULL || IsA(a, Bitmapset)); + Assert(BITMAPSET_IS_VALID(a)); /* do nothing if nothing is called for, without further checking */ if (upper < lower) @@ -1037,11 +1037,8 @@ bms_int_members(Bitmapset *a, const Bitmapset *b) Bitmapset *tmp = a; #endif - Assert(a == NULL || IsA(a, Bitmapset)); - Assert(b == NULL || IsA(b, Bitmapset)); - - Assert(a == NULL || IsA(a, Bitmapset)); - Assert(b == NULL || IsA(b, Bitmapset)); + Assert(BITMAPSET_IS_VALID(a)); + Assert(BITMAPSET_IS_VALID(b)); /* Handle cases where either input is NULL */ if (a == NULL) @@ -1093,8 +1090,8 @@ bms_del_members(Bitmapset *a, const Bitmapset *b) Bitmapset *tmp = a; #endif - Assert(a == NULL || (IsA(a, Bitmapset) && a->words[a->nwords - 1] != 0)); - Assert(b == NULL || (IsA(b, Bitmapset) && b->words[b->nwords - 1] != 0)); + Assert(BITMAPSET_IS_VALID(a)); + Assert(BITMAPSET_IS_VALID(b)); /* Handle cases where either input is NULL */ if (a == NULL) @@ -1164,11 +1161,8 @@ bms_join(Bitmapset *a, Bitmapset *b) Bitmapset *tmp = a; #endif - Assert(a == NULL || IsA(a, Bitmapset)); - Assert(b == NULL || IsA(b, Bitmapset)); - - Assert(a == NULL || IsA(a, Bitmapset)); - Assert(b == NULL || IsA(b, Bitmapset)); + Assert(BITMAPSET_IS_VALID(a)); + Assert(BITMAPSET_IS_VALID(b)); /* Handle cases where either input is NULL */ if (a == NULL) @@ -1231,7 +1225,7 @@ bms_next_member(const Bitmapset *a, int prevbit) int wordnum; bitmapword mask; - Assert(a == NULL || IsA(a, Bitmapset)); + Assert(BITMAPSET_IS_VALID(a)); if (a == NULL) return -2; @@ -1292,7 +1286,7 @@ bms_prev_member(const Bitmapset *a, int prevbit) int ushiftbits; bitmapword mask; - Assert(a == NULL || IsA(a, Bitmapset)); + Assert(BITMAPSET_IS_VALID(a)); /* * If set is NULL or if there are no more bits to the right then we've -- 2.31.0