On Wed, 20 Mar 2019 16:32:25 +0000
Matt Helsley <mhelsley@xxxxxxxxxx> wrote:
> > +/**
> > + * tep_check_flag - check the state of event parser flag
> > + * @tep: a handle to the tep_handle
> > + * @flag: flag, or combination of flags to be checked
> > + * can be any combination from enum tep_flag
> > + *
> > + * This checks the state of a flag or combination of flags from enum tep_flag
> > + */
> > +int tep_check_flag(struct tep_handle *tep, enum tep_flag flag)
> > +{
> > + if (tep)
> > + return (tep->flags & flag);
> > + return 0;
> > +}
>
> This returns a subset of the flags directly — it doesn’t really check them -- that’s up to the caller.
> So I’d say this is more of a “getter" than a “checker”.
If we want to be consistent with the kernel, the proper name is "test"
tep_test_flag()
>
> If returning a “boolean” is the true intent of the API then it should be:
>
> return (tep->flags & flag) == flag;
Hmm, this has some side effects that would require documentation. It
makes it only return true if all flags are set, which is not what we
would want.
Since we are only dealing with one flag, (the parameter is an enum, so
multiple flags would not be correct), keeping the return value as is is
the proper approach.
But I'm fine with making the return value a bool and rename it to
"tep_test_flag()"
-- Steve
|