On Wed, Jun 30, 2021 at 10:52 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Jun 29, 2021 at 9:41 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > On 2021-Jun-29, Bharath Rupireddy wrote:
> >
> > > On Tue, Jun 29, 2021 at 4:37 PM Amit Kapila <amit.kapila16@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.