On Sep 23 2025, at 12:46 pm, Tom Lane <t...@sss.pgh.pa.us> wrote: > Michael Paquier <mich...@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.
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
v1-0001-Avoid-triggering-warnings-in-Bitmapset-tests-rela.patch
Description: Binary data
