Обсуждение: Remove PointerIsValid()

I think there is agreement that the PointerIsValid() macro is pretty 
useless.  This patch proposes to remove it.  There have been a few 
recent mini-discussions in other threads that appear to support this. [0][1]

There were the usual concerns about code churn and backpatching and so 
on, but I think in the end the change is not that big and it's in pretty 
boring positions.  Also, you can backpatch code without PointerIsValid() 
just fine.  You might run into trouble if you forward-patch. :-/

While converting the code, I tried to find a balance of style of

     if (PointerIsValid(foo))

to

     if (foo)

or

     if (foo != NULL)

but there is no deterministic reason.

(Note that when you convert the first form to the third form, you have 
to flip the overall sense of the logic, which might look confusing in 
some places.)


[0]: 
https://www.postgresql.org/message-id/CA+hUKG+NFKnr=K4oybwDvT35dW=VAjAAfiuLxp+5JeZSOV3nBg@mail.gmail.com
[1]: 
https://www.postgresql.org/message-id/bccf2803-5252-47c2-9ff0-340502d5bd1c@iki.fi

On Wed, Sep 17, 2025 at 1:21 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> I think there is agreement that the PointerIsValid() macro is pretty
> useless.  This patch proposes to remove it.  There have been a few
> recent mini-discussions in other threads that appear to support this. [0][1]

I'm also in favor of removing it.

> There were the usual concerns about code churn and backpatching and so
> on, but I think in the end the change is not that big and it's in pretty
> boring positions.

I agree.

--
Peter Geoghegan

On Wed, Sep 17, 2025 at 4:12 AM Chao Li <li.evan.chao@gmail.com> wrote:
> Given the context of replacing PointerIsValid(x), I think if (foo != NULL) is slightly better than if (x), because
thatexplicitly shows the intent of checking pointers, while if (x) works for both pointers and integers. 

I agree that we should prefer foo != NULL, but if the surrounding code
in a particular location just tests if (foo), then it may be better in
that case to go that route.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Sep 17, 2025 at 10:15:10AM -0400, Peter Geoghegan wrote:
> On Wed, Sep 17, 2025 at 1:21 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>> I think there is agreement that the PointerIsValid() macro is pretty
>> useless.  This patch proposes to remove it.  There have been a few
>> recent mini-discussions in other threads that appear to support this. [0][1]
> 
> I'm also in favor of removing it.
> 
>> There were the usual concerns about code churn and backpatching and so
>> on, but I think in the end the change is not that big and it's in pretty
>> boring positions.
> 
> I agree.

+1

-- 
nathan

On Tue, Sep 16, 2025 at 10:21 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> I think there is agreement that the PointerIsValid() macro is pretty
> useless.  This patch proposes to remove it.  There have been a few
> recent mini-discussions in other threads that appear to support this. [0][1]

Patch LGTM, and I like your chosen balance of the two replacements.

On Wed, Sep 17, 2025 at 8:55 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Wed, Sep 17, 2025 at 4:12 AM Chao Li <li.evan.chao@gmail.com> wrote:
> > Given the context of replacing PointerIsValid(x), I think if (foo != NULL) is slightly better than if (x), because
thatexplicitly shows the intent of checking pointers, while if (x) works for both pointers and integers. 
>
> I agree that we should prefer foo != NULL, but if the surrounding code
> in a particular location just tests if (foo), then it may be better in
> that case to go that route.

(I find `if (some_pointer)` to be very readable, idiomatic C, so I
would vote against standardization.)

--Jacob

On Thu, 18 Sept 2025 at 03:55, Robert Haas <robertmhaas@gmail.com> wrote:
> I agree that we should prefer foo != NULL, but if the surrounding code
> in a particular location just tests if (foo), then it may be better in
> that case to go that route.

+1. I generally do (var != NULL) for pointers rather than (var). I
think it makes the code easier to read as a reader instantly knows
"var" is a pointer and not an int or bool.

Also, agree that we can relax that a little when all the surrounding
code does (var)

David

On Wed, Sep 17, 2025 at 11:21:10AM -0500, Nathan Bossart wrote:
> On Wed, Sep 17, 2025 at 10:15:10AM -0400, Peter Geoghegan wrote:
>>> There were the usual concerns about code churn and backpatching and so
>>> on, but I think in the end the change is not that big and it's in pretty
>>> boring positions.
>>
>> I agree.
>
> +1

The locations patched don't really worry me, so the removal of the
macro is fine for me.
--
Michael

On 17.09.25 18:26, Jacob Champion wrote:
> On Tue, Sep 16, 2025 at 10:21 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>>
>> I think there is agreement that the PointerIsValid() macro is pretty
>> useless.  This patch proposes to remove it.  There have been a few
>> recent mini-discussions in other threads that appear to support this. [0][1]
> 
> Patch LGTM, and I like your chosen balance of the two replacements.

Thanks, pushed as is.  (Some people appeared to have stylistic 
preferences one way or another, but there was nothing specific proposed. 
  Follow-up patches could be entertained, of course.)