Re: Enumize logical replication message actions

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.

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)

===

[1] https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wswitch

Kind Regards,
Peter Smith.
Fujitsu Australia