Re: [Xen-devel] [PATCH 2/3] nodemask: Remove implicit addressof from the API

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.06.19 at 16:43, <andrew.cooper3@xxxxxxxxxx> wrote:
> The nodemask API differs from the cpumask API because each wrapper to bitmap
> operations is further wrapped by a macro which takes the address of the
> nodemask objects.
> 
> This results in code which is slightly confusing to read as it doesn't follow
> C's calling conventions, and prohibits the use of slightly more complicated
> constructs for specifying parameters.
> 
> Drop all wrapping macros, rename the nodemask static inline functions to drop
> the double underscores, and feed MAX_NUMNODES into appropriate locations.
> 
> Take the opportunity to drop a compiler workaround for node_isset() for GCC
> 3.3.2 which is long out of support, and implment it with a static inline.
> 
> Update all callers to use the correct indirection themselves.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

I'm okay with this in principle, so
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
(with one aspect addressed below), but to be honest I would
have hoped that the switch to the cpumask.h model would also
imply a switch to the naming used there (e.g. nodemask_and()).
This would have provided the opportunity to not do the entire
switch in one patch.

> -/* No static inline type checking - see Subtlety (1) above. */
> -#define node_isset(node, nodemask) test_bit((node), (nodemask).bits)
> +static inline int node_isset(int node, const nodemask_t *src)
> +{
> +     return test_bit(node, src->bits);
> +}

Since this is a new function, could I ask that you make it return bool?
(Same for the test_and_... and a few others below then.) And (also
elsewhere) could I further ask that plain int be switched to unsigned
int at this occasion?

> -#define nodes_shift_right(dst, src, n) \
> -                     __nodes_shift_right(&(dst), &(src), (n), MAX_NUMNODES)
> -static inline void __nodes_shift_right(nodemask_t *dstp,
> -                                     const nodemask_t *srcp, int n, int 
> nbits)
> +static inline void nodes_shift_right(nodemask_t *dstp, const nodemask_t 
> *srcp,
> +                                  int n)
>  {
> -     bitmap_shift_right(dstp->bits, srcp->bits, n, nbits);
> +     bitmap_shift_right(dstp->bits, srcp->bits, n, MAX_NUMNODES);
>  }
>  
> -#define nodes_shift_left(dst, src, n) \
> -                     __nodes_shift_left(&(dst), &(src), (n), MAX_NUMNODES)
> -static inline void __nodes_shift_left(nodemask_t *dstp,
> -                                     const nodemask_t *srcp, int n, int 
> nbits)
> +static inline void nodes_shift_left(nodemask_t *dstp, const nodemask_t *srcp,
> +                                 int n)
>  {
> -     bitmap_shift_left(dstp->bits, srcp->bits, n, nbits);
> +     bitmap_shift_left(dstp->bits, srcp->bits, n, MAX_NUMNODES);
>  }

How about ditching rather than adjusting these two?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel