Horton

Thread

Commits

  1. Remove PointerIsValid()

  1. Remove PointerIsValid()

    Peter Eisentraut <peter@eisentraut.org> — 2025-09-17T05:21:22Z

    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

  2. Re: Remove PointerIsValid()

    Chao Li <li.evan.chao@gmail.com> — 2025-09-17T08:11:40Z

    
> On Sep 17, 2025, at 13:21, 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]
> 
> 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<0001-Remove-PointerIsValid.patch>


Given the context of replacing PointerIsValid(x), I think if (foo != NULL) is slightly better than if (x), because that explicitly shows the intent of checking pointers, while if (x) works for both pointers and integers.

In this patch, you mix using the both forms, should we just use a single form?

Regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  3. Re: Remove PointerIsValid()

    Peter Geoghegan <pg@bowt.ie> — 2025-09-17T14:15:10Z

    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

  4. Re: Remove PointerIsValid()

    Robert Haas <robertmhaas@gmail.com> — 2025-09-17T15:54:46Z

    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 that explicitly 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

  5. Re: Remove PointerIsValid()

    Nathan Bossart <nathandbossart@gmail.com> — 2025-09-17T16:21:10Z

    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

  6. Re: Remove PointerIsValid()

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-09-17T16:26:25Z

    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 that explicitly 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

  7. Re: Remove PointerIsValid()

    David Rowley <dgrowleyml@gmail.com> — 2025-09-17T20:23:46Z

    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

  8. Re: Remove PointerIsValid()

    Michael Paquier <michael@paquier.xyz> — 2025-09-17T22:36:03Z

    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

  9. Re: Remove PointerIsValid()

    Peter Eisentraut <peter@eisentraut.org> — 2025-09-24T15:05:02Z

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