Re: [PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS
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 v2-0001-Use-macro-to-define-the-number-of-enum-values.patch Description: Binary data
Re: [PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS
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.
Re: [PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS
On Mon, 22 Apr 2024 at 06:04, Michael Paquier 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
Re: [PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS
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 signature.asc Description: PGP signature
Re: [PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS
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 v1-0001-Use-macro-to-define-the-number-of-enum-values.patch Description: Binary data
Re: [PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS
On Thu, 18 Apr 2024 at 13:00, Aleksander Alekseev wrote: > > Fair point. PFA the alternative version of the patch. > Thanks. Committed. Regards, Dean
Re: [PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS
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 v2-0001-Replace-constant-3-with-NUM_MERGE_MATCH_KINDS.patch Description: Binary data
Re: [PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS
On Tue, 16 Apr 2024 at 11:35, Richard Guo wrote: > > On Tue, Apr 16, 2024 at 5:48 PM Aleksander Alekseev > 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
Re: [PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS
On Tue, Apr 16, 2024 at 5:48 PM Aleksander Alekseev < aleksan...@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
[PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS
Oversight of 0294df2f1f84 [1]. [1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=0294df2f1f84 -- Best regards, Aleksander Alekseev v1-0001-Replace-constant-3-with-NUM_MERGE_MATCH_KINDS.patch Description: Binary data
