From: Tom Lane <tgl@sss.pgh.pa.us>
To: Michael Paquier <michael@paquier.xyz>
cc: Greg Burd <greg@burd.me>,
        PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
        Robert Haas <robertmhaas@gmail.com>,
        Nathan Bossart <nathandbossart@gmail.com>,
        Masahiko Sawada <sawada.mshk@gmail.com>
Subject: Re: [PATCH] Add tests for Bitmapset
In-reply-to: <aNJ_YTuFnMn23PEI@paquier.xyz>
References: <aMz57nHgkLozddYu@paquier.xyz>
 <CD368B71-2947-407D-8967-181A4CB4A985@greg.burd.me>
 <aNEBUeYqqSVOSVaP@paquier.xyz> <3027069.1758606227@sss.pgh.pa.us>
 <aNJ_YTuFnMn23PEI@paquier.xyz>
Comments: In-reply-to Michael Paquier <michael@paquier.xyz>
	message dated "Tue, 23 Sep 2025 20:07:13 +0900"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3097369.1758646009.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Tue, 23 Sep 2025 12:46:49 -0400
Message-ID: <3097370.1758646009@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3097370.1758646009%40sss.pgh.pa.us>
Precedence: bulk

Michael Paquier <michael@paquier.xyz> 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.

> 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)))

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_cstrin=
g(str)))

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