Re: *_LAST in enums to define NUM* macross

From: Roman Khapov <rkhapov(at)yandex-team(dot)ru>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: *_LAST in enums to define NUM* macross
Date: 2025-11-18 18:46:15
Message-ID: 262BF411-469E-407B-96C7-D55B2411B173@yandex-team.ru
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Thanks for your feedback!

> On 18 Nov 2025, at 21:49, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Isn't this patch basically proposing to revert 10b7218?
> How has the situation changed since then to make that
> a good idea?

No, it is not a revert, until we don’t have a separated ENUMFOO_NUM value with numeric value as <last> + 1, this patch offers similar, but still slightly different way to define that value..

I was hoping that when adding new values, the developer would think "why there is a ENUMFOO_LAST here" and leave it the last value of the enum, allowing ENUMFOO_NUM to stay correct after adding new values. Just as it was explicitly stated before pg18 in comment in enum definition, but at the same time I wanted to get rid of the compiler warnings.

>
>> Please note, that this will not lead to extra compiler warnings about writing switch statements on enum values,
>> because LAST value in enums is always equal to some other value of the enum.
>
> TBH I find that argument wildly optimistic. Even if it's true
> today it could stop being true next week.
>

Hmm, I don’t see any reason, why compilers can add warnings about missing enum value, that int representation equals to some other enum's value. It would be an correct warning if you forget to write case for value, that is equals to ENUMFOO_LAST, or it would be a bug in compiler, if it offers you to write case statements that is already covered some other case statement.

> PMSIGNAL_BACKGROUND_WORKER_CHANGE, /* background worker state change */
> PMSIGNAL_START_WALRECEIVER, /* start a walreceiver */
> PMSIGNAL_ADVANCE_STATE_MACHINE, /* advance postmaster's state machine */
> PMSIGNAL_XLOG_IS_SHUTDOWN, /* ShutdownXLOG() completed */
> +
> +#define NUM_PMSIGNALS (PMSIGNAL_XLOG_IS_SHUTDOWN+1)
> } PMSignalReason;
> -
> -#define NUM_PMSIGNALS (PMSIGNAL_XLOG_IS_SHUTDOWN+1)

Anyway, I like your variant of defining ENUMFOO_NUM inside enum, because it allows to achieve (or at least try to (: ) goal of making developers think smth like "why there is that thing” and not to break ENUMFOO_NUM accidentally when they are adding new ENUMFOO_VALUE in the end of the enum.

> (I'm not actually advocating such a patch, because I doubt it's
> worth the trouble. But perhaps others will think it worthwhile.)

Still, I made a patch v2, in case someone decides that it would be useful.

Attachment Content-Type Size
v2-0001-move-ENUMFOO_NUM-definitions-inside-enums.patch application/octet-stream 4.3 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Melanie Plageman 2025-11-18 18:49:28 Re: Checkpointer write combining
Previous Message David Geier 2025-11-18 18:35:32 Re: Performance issues with parallelism and LIMIT