On Wed, Jun 2, 2021 at 3:33 PM Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> wrote: > > > + /* If connect option is supported, the others also need to be. */ > > + Assert((supported_opts & SUBOPT_CONNECT) == 0 || > > + ((supported_opts & SUBOPT_ENABLED) != 0 && > > + (supported_opts & SUBOPT_CREATE_SLOT) != 0 && > > + (supported_opts & SUBOPT_COPY_DATA) != 0)); > > + > > + /* Set default values for the supported options. */ > > + if ((supported_opts & SUBOPT_CONNECT) != 0) > > + vals->connect = true; > > + > > + if ((supported_opts & SUBOPT_ENABLED) != 0) > > + vals->enabled = true; > > + > > + if ((supported_opts & SUBOPT_CREATE_SLOT) != 0) > > + vals->create_slot = true; > > + > > + if ((supported_opts & SUBOPT_SLOT_NAME) != 0) > > + vals->slot_name = NULL; > > + > > + if ((supported_opts & SUBOPT_COPY_DATA) != 0) > > + vals->copy_data = true; > > > > 3. Are all those "!= 0" really necessary when checking the > > supported_opts against the bit masks? Maybe it is just a style thing, > > but since there are so many of them I felt it contributed to clutter > > and made the code less readable. This pattern was in many places, not > > just the example above. > > Yeah these are necessary to know whether a particular option's bit is > set in the bitmask.
Hmmm. Maybe I did not ask the question properly. See below. > How about having a macro like below: > #define IsSet(val, option) ((val & option) != 0) > The if statements can become like below: > if (IsSet(supported_opts, SUBOPT_CONNECT)) > if (IsSet(supported_opts, SUBOPT_ENABLED)) > if (IsSet(supported_opts, SUBOPT_SLOT_NAME)) > if (IsSet(supported_opts, SUBOPT_COPY_DATA)) > > The above looks better to me. Thoughts? Yes, it looks better, but (since the masks are all 1 bit) I was only asking why not do like: if (supported_opts & SUBOPT_CONNECT) if (supported_opts & SUBOPT_ENABLED) if (supported_opts & SUBOPT_SLOT_NAME) if (supported_opts & SUBOPT_COPY_DATA) > > Can we implement a similar idea for the parse_publication_options > although there are only two options right now. Option parsing code > will be consistent for logical replication DDLs and is extensible. > Thoughts? I have no strong opinion about it. It seems a trade off between having a goal of "code consistency", versus "if it aint broke don't fix it". ------ Kind Regards, Peter Smith. Fujitsu Australia
