On Wed, Feb 19, 2020 at 3:54 PM jwcart2 <jwcart2@xxxxxxxxxxxxx> wrote:
> On 2/19/20 3:25 AM, Ondrej Mosnacek wrote:
> > On Tue, Feb 18, 2020 at 9:45 PM James Carter <jwcart2@xxxxxxxxxxxxx> wrote:
> >> When checking whether or not an ebitmap has any bits set, use
> >> ebitmap_length() instead of ebitmap_cardinality().
> >>
> >> There is no need to find out how many bits are set, if all that is
> >> needed is to determine if any bits are set at all.
> >>
> >> Signed-off-by: James Carter <jwcart2@xxxxxxxxxxxxx>
> >> ---
> >> libsepol/src/kernel_to_cil.c | 10 +++++-----
> >> libsepol/src/kernel_to_conf.c | 8 ++++----
> >> libsepol/src/module_to_cil.c | 16 ++++++++--------
> >> 3 files changed, 17 insertions(+), 17 deletions(-)
> >
> > Thanks, this looks good! Although I'm thinking if we shouldn't add a
> > specific function for this, e.g.:
> >
> > static inline bool ebitmap_is_empty(ebitmap_t *e)
> > {
> > return ebitmap_length(e) == 0;
> > }
> >
> > ...because ebitmap_length() is kind of an implementation detail and it
> > is easy to confuse ebitmap_length() and ebitmap_cardinality(). Note
> > there are already some existing callers of ebitmap_length() that would
> > also need converting to ebitmap_is_empty() in that case.
> >
> > <diff snipped>
> >
>
> I think ebitmap_is_empty() is a good idea, but I think a macro will work fine.
>
> #define ebitmap_is_empty(e) (((e)->highbit) == 0)
I personally try to use inline functions rather than macros whenever
possible - they are type-checked and consistent with the rest of the C
code. Using macro where a function would suffice is kind of a hack,
IMO. But in this very simple case it doesn't make much practical
difference, so I'll leave it to your decision (unless other
maintainers also object).
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
