Re: Refactor to make use of a common function for GetSubscriptionRelations and GetSubscriptionNotReadyRelations.

On Thu, Jul 21, 2022 at 7:10 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Wed, Jul 20, 2022 at 02:32:44PM +0530, Amit Kapila wrote:
> > On Thu, Jul 14, 2022 at 4:34 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >> By using a bitmask I think there is an implication that the flags can
> >> be combined...
> >>
> >> Perhaps it is not a problem today, but later you may want more flags. e.g.
> >> #define SUBSCRIPTION_REL_STATE_READY 0x02 /* READY relations */
> >>
> >> then the bitmask idea falls apart because IIUC you have no intentions
> >> to permit things like:
> >> (SUBSCRIPTION_REL_STATE_NOT_READY | SUBSCRIPTION_REL_STATE_READY)
> >
> > I think this will be an invalid combination if caller ever used it.
> > However, one might need to use a combination like
> > (SUBSCRIPTION_REL_STATE_READY | SUBSCRIPTION_REL_STATE_DONE). For such
> > cases, I feel the bitmask idea will be better.
>
> It feels unnatural to me to have a flag saying "not-ready" and one
> saying "ready", while we could have a flag saying "ready" that can be
> combined with a second flag to decide if the contents of srsubstate
> should be matched or *not* matched with the states expected by the
> caller. This could be extended to more state values, for example.
>
> I am not sure if we actually need this much as I have no idea if
> future features would use it, so please take my suggestion lightly :)
>

Yeah, it is not very clear to me either. I think this won't be
difficult to change one or another way depending on future needs. At
this stage, it appeared to me that bitmask is a better way to
represent this information but if you and other feels using enum is a
better idea then I am fine with that as well.

--
With Regards,
Amit Kapila.