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.
--
Álvaro Herrera Valdivia, Chile
"Right now the sectors on the hard disk run clockwise, but I heard a rumor that
you can squeeze 0.2% more throughput by running them counterclockwise.
It's worth the effort. Recommended." (Gerry Pourwelle)