On Wed, Jun 30, 2021 at 10:52 AM Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Tue, Jun 29, 2021 at 9:41 PM Alvaro Herrera <alvhe...@alvh.no-ip.org>
> wrote:
> >
> > On 2021-Jun-29, Bharath Rupireddy wrote:
> >
> > > On Tue, Jun 29, 2021 at 4:37 PM Amit Kapila <amit.kapil...@gmail.com>
> > > wrote:
> > > > Few comments:
> > > > ===============
> > > > 1.
> > > > +typedef struct SubOpts
> > > > +{
> > > > + bits32 supported_opts; /* bitmap of supported SUBOPT_* */
> > > > + bits32 specified_opts; /* bitmap of user specified SUBOPT_* */
> > > >
> > > > I think it will be better to not keep these as part of this structure.
> > > > Is there a reason for doing so?
> > >
> > > I wanted to pack all the parsing related params passed to
> > > parse_subscription_options into a single structure since this is one
> > > of the main points (reducing the number of function params) on which
> > > the patch is coded.
> >
> > Yeah I was looking at the struct too and this bit didn't seem great. I
> > think it'd be better to have the struct be output only; so
> > "specified_opts" would be part of the struct (not necessarily with that
> > name), but "supported opts" (which is input data) would be passed as a
> > separate argument. That seems cleaner to *me*, at least.
> >
>
> Yeah, that sounds better than what we have in the patch. Also, I am
> not sure if it is a good idea to use "supported_opts" for input data
> as that sounds more like what is output from the function, how about
> required_opts or input_opts? Also, we can name the output structure as
> SpecifiedSubOpts and "specified_opts" as either "opts" or "out_opts".IMO, SubOpts looks okay. Also, I retained the specified_opts but moved supported_opts out of the structure. > I think naming these things is a bit matter of personal preference so > I am fine if both you and Bharath find current naming more meaningful. Please let me know if any of the names look odd. > +#define IsSet(val, bits) ((val & (bits)) == (bits)) > Also, do you have any opinion on this define? I see at other places we > use in-place checks but as in this patch there are multiple instances > of such check so probably such a define should be acceptable. Yeah. I'm retaining this macro as it makes code readable. PFA v9 patch set for further review. With Regards, Bharath Rupireddy.
v9-0001-Refactor-parse_subscription_options.patch
Description: Binary data
v9-0002-Remove-similar-ereport-calls-in-parse_subscriptio.patch
Description: Binary data
