Horton

Thread

Commits

  1. Use macro to define the number of enum values

  2. Use macro NUM_MERGE_MATCH_KINDS instead of '3' in MERGE code.

  1. [PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS

    Aleksander Alekseev <aleksander@timescale.com> — 2024-04-16T09:48:23Z

    Oversight of 0294df2f1f84 [1].

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

-- 
Best regards,
Aleksander Alekseev

  2. Re: [PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS

    Richard Guo <guofenglinux@gmail.com> — 2024-04-16T10:35:19Z

    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

  3. Re: [PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2024-04-16T13:21:40Z

    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

  4. Re: [PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS

    Aleksander Alekseev <aleksander@timescale.com> — 2024-04-18T12:00:09Z

    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

  5. Re: [PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2024-04-19T09:02:42Z

    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

  6. Re: [PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS

    Aleksander Alekseev <aleksander@timescale.com> — 2024-04-19T09:47:43Z

    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

  7. Re: [PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS

    Michael Paquier <michael@paquier.xyz> — 2024-04-22T05:04:04Z

    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

  8. Re: [PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2024-04-22T07:55:37Z

    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

  9. Re: [PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS

    Peter Eisentraut <peter@eisentraut.org> — 2024-05-12T11:57:43Z

    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.

  10. Re: [PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS

    Aleksander Alekseev <aleksander@timescale.com> — 2024-05-13T10:22:22Z

    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

  11. Re: [PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS

    Peter Eisentraut <peter@eisentraut.org> — 2024-10-01T13:46:45Z

    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