Re: Enumize logical replication message actions

On Fri, Oct 30, 2020 at 10:37 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Fri, Oct 30, 2020 at 2:46 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> Hi Amit
>
> > You mentioned in the beginning that you prefer to use Enum instead of
> > define so that switch cases can detect any remaining items but I have
> > tried adding extra enum value at the end and didn't handle that in
> > switch case but I didn't get any compilation warning or error. Do we
> > need something else to detect that at compile time?
>
> See [1] some GCC compiler options that can expose missing cases like those.
>

Thanks, I am able to see the warnings now.

> e.g. use -Wswitch or -Wswitch-enum
> Detection depends if the switch has a default case or not.
>
> > 4.
> > switch (action)
> > {
> > - /* BEGIN */
> > - case 'B':
> > + case LOGICAL_REP_MSG_BEGIN:
> > apply_handle_begin(s);
> > - break;
> > - /* COMMIT */
> > - case 'C':
> > + return;
> >
> > I think we can simply use 'return apply_handle_begin;' instead of
> > adding return in another line. Again, I think we changed this handling
> > in apply_dispatch() to improve the case where we can detect at the
> > compile time any missing enum but at this stage it is not clear to me
> > if that is true.
>
> IIUC getting rid of the default from the switch can make the missing
> enum detection easier because then you can use -Wswitch option to
> expose the problem (instead of -Wswitch-enum which may give lots of
> false positives as well)
>

Fair enough. So, it makes sense to move the default out of the switch case.

Ashutosh, see if we can add in comments (or may be commit message) why
we preferred to use enum for these messages.

--
With Regards,
Amit Kapila.