public inbox for [email protected]  

help / color / mirror / Atom feed

From: Greg Burd <[email protected]>
To: Tom Lane <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: [PATCH] Add tests for Bitmapset
Date: Tue, 23 Sep 2025 14:26:19 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>


On Sep 23 2025, at 12:46 pm, Tom Lane <[email protected]> wrote:

> Michael Paquier <[email protected]> writes:
>> On Tue, Sep 23, 2025 at 01:43:47AM -0400, Tom Lane wrote:
>>> This patch seems to be rather full of arbitrary casts to or
>>> from Datum, which is no longer okay.  You need to be using
>>> the appropriate conversion macros, such as PointerGetDatum.

Tom, thanks for pointing out the oversight.  Apologies, I should have
tried that out in my local tests.

>> Right, this can ve reproduced with a -m32 added to gcc.
> 
> Curiously, I don't see these warnings on 32-bit FreeBSD
> (with clang version 19.1.7).  But I tested your patch on
> mamba's host and confirmed that it makes that gcc happy.
> 
>> I don't see a need for a Datum manipulation in these conversion
>> macros, as we already allocate the results to and from "text"
>> before/after using the GETARG or RETURN macros.  Using directly
>> text_to_cstring() and cstring_to_text() takes care of the warnings, as
>> well.
> 
> Yeah, this is better, but I think the new macros could use a bit
> more parenthesis-twiddling:
> 
> +#define BITMAPSET_TO_TEXT(bms) cstring_to_text(nodeToString((bms)))

Agreed, that was my mistake.

> The extra parens around "bms" are surely unnecessary.  (If they were
> necessary, it could only be because nodeToString was a macro that
> failed to parenthesize its argument where needed.  But that would
> be a bug to fix in nodeToString, not here.)
> 
> +#define TEXT_TO_BITMAPSET(str) (Bitmapset *) stringToNode(text_to_cstring(str))
> 
> Here, on the other hand, I think an extra outer set of parens
> would be a good idea, ie
> 
> +#define TEXT_TO_BITMAPSET(str) ((Bitmapset *) stringToNode(text_to_cstring(str)))

Sounds reasonable to me.

> It may be that the C cast binds tightly enough that this cannot
> be a problem, but I don't think assuming that is project style.
> 
> 			regards, tom lane

patch attached, best.

-greg


Attachments:

  [application/octet-stream] v1-0001-Avoid-triggering-warnings-in-Bitmapset-tests-rela.patch (1.5K, 2-v1-0001-Avoid-triggering-warnings-in-Bitmapset-tests-rela.patch)
  download | inline diff:
From ca3031bf1b041839bf629ea44f58371796e9ebd5 Mon Sep 17 00:00:00 2001
From: Greg Burd <[email protected]>
Date: Tue, 23 Sep 2025 14:11:10 -0400
Subject: [PATCH v1] Avoid triggering warnings in Bitmapset tests related to
 Datum casts

The Bitmapset tests encode/decode bitmaps to/from text using the available
nodeTo/FromString() functions for simplicity.  They don't need for a Datum
manipulation in their conversion macros, as we already allocate the results to
and from "text" before/after using the GETARG or RETURN macros. Using directly
text_to_cstring() and cstring_to_text() takes care of these warnings.
---
 src/test/modules/test_bitmapset/test_bitmapset.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/test/modules/test_bitmapset/test_bitmapset.c b/src/test/modules/test_bitmapset/test_bitmapset.c
index 61f256f65a4..5bc4daa23f1 100644
--- a/src/test/modules/test_bitmapset/test_bitmapset.c
+++ b/src/test/modules/test_bitmapset/test_bitmapset.c
@@ -84,8 +84,8 @@ PG_FUNCTION_INFO_V1(test_random_operations);
 	} while (0)
 
 /* Encode/Decode to/from TEXT and Bitmapset */
-#define BITMAPSET_TO_TEXT(bms) (text *) CStringGetTextDatum(nodeToString((bms)))
-#define TEXT_TO_BITMAPSET(str) (Bitmapset *) stringToNode(TextDatumGetCString((Datum) (str)))
+#define BITMAPSET_TO_TEXT(bms) cstring_to_text(nodeToString(bms))
+#define TEXT_TO_BITMAPSET(str) ((Bitmapset *) stringToNode(text_to_cstring(str)))
 
 /*
  * Individual test functions for each bitmapset API function
-- 
2.50.1 (Apple Git-155)

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