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".
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.
+#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.
--
With Regards,
Amit Kapila.