[PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS

On Tue, Apr 16, 2024 at 5:48 PM Aleksander Alekseev <
aleksander@timescale.com> wrote:

Oversight of 0294df2f1f84 [1].

[1]:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=0294df2f1f84

+1. I think this change improves the code quality. I searched for
other arrays indexed by merge match kind, but found none. So this patch
seems thorough.

Thanks
Richard

On Tue, 16 Apr 2024 at 11:35, Richard Guo <guofenglinux@gmail.com> wrote:

On Tue, Apr 16, 2024 at 5:48 PM Aleksander Alekseev <aleksander@timescale.com> wrote:

Oversight of 0294df2f1f84 [1].

[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=0294df2f1f84

+1. I think this change improves the code quality. I searched for
other arrays indexed by merge match kind, but found none. So this patch
seems thorough.

Yes this makes sense, though I note that some other similar code uses
a #define rather than inserting an enum element at the end (e.g.,
NUM_ROWFILTER_PUBACTIONS).

I guess the argument against inserting an enum element at the end is
that a switch statement on the enum value might generate a compiler
warning if it didn't have a default clause.

Looking at how NUM_ROWFILTER_PUBACTIONS is defined as the last element
plus one, it might seem to be barely any better than just defining it
to be 3, since any new enum element would probably be added at the
end, requiring it to be updated in any case. But if the number of
elements were much larger, it would be much more obviously correct,
making it a good general pattern to follow. So in the interests of
code consistency, I think we should do the same here.

Regards,
Dean

Hi,

I guess the argument against inserting an enum element at the end is
that a switch statement on the enum value might generate a compiler
warning if it didn't have a default clause.

Fair point. PFA the alternative version of the patch.

--
Best regards,
Aleksander Alekseev

On Thu, 18 Apr 2024 at 13:00, Aleksander Alekseev
<aleksander@timescale.com> wrote:

Fair point. PFA the alternative version of the patch.

Thanks. Committed.

Regards,
Dean

Hi,

Fair point. PFA the alternative version of the patch.

Thanks. Committed.

Thanks. I see a few pieces of code that use special FOO_NUMBER enum
values instead of a macro. Should we refactor these pieces
accordingly? PFA another patch.

--
Best regards,
Aleksander Alekseev

On Fri, Apr 19, 2024 at 12:47:43PM +0300, Aleksander Alekseev wrote:

Thanks. I see a few pieces of code that use special FOO_NUMBER enum
values instead of a macro. Should we refactor these pieces
accordingly? PFA another patch.

I don't see why not for the places you are changing here, we can be
more consistent. Now, such changes are material for v18.
--
Michael

On Mon, 22 Apr 2024 at 06:04, Michael Paquier <michael@paquier.xyz> wrote:

On Fri, Apr 19, 2024 at 12:47:43PM +0300, Aleksander Alekseev wrote:

Thanks. I see a few pieces of code that use special FOO_NUMBER enum
values instead of a macro. Should we refactor these pieces
accordingly? PFA another patch.

I don't see why not for the places you are changing here, we can be
more consistent.

[Shrug] I do prefer using a macro. Adding a counter element to the end
of the enum feels like a hack, because the counter isn't the same kind
of thing as all the other enum elements, so it feels out of place in
the enum. On the other hand, I think it's a fairly common pattern that
most people will recognise, and for other enums that are more likely
to grow over time, it might be less error-prone than a macro, which
people might overlook and fail to update.

Now, such changes are material for v18.

Agreed. This has been added to the next commitfest, so let's see what
others think.

Regards,
Dean

On 19.04.24 11:47, Aleksander Alekseev wrote:

Thanks. I see a few pieces of code that use special FOO_NUMBER enum
values instead of a macro. Should we refactor these pieces
accordingly? PFA another patch.

I think this is a sensible improvement.

But please keep the trailing commas on the last enum items.

Hi,

Thanks. I see a few pieces of code that use special FOO_NUMBER enum
values instead of a macro. Should we refactor these pieces
accordingly? PFA another patch.

I think this is a sensible improvement.

But please keep the trailing commas on the last enum items.

Thanks, fixed.

--
Best regards,
Aleksander Alekseev

On 13.05.24 12:22, Aleksander Alekseev wrote:

Hi,

Thanks. I see a few pieces of code that use special FOO_NUMBER enum
values instead of a macro. Should we refactor these pieces
accordingly? PFA another patch.

I think this is a sensible improvement.

But please keep the trailing commas on the last enum items.

Thanks, fixed.

committed