[tarantool-patches] Re: [PATCH v1 1/4] box: introduce new helpers in column_mask.h

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch! See 2 comments below.

On 08/02/2019 13:52, Kirill Shcherbatov wrote:
> Refactored column_mask.h definitions: introduced a new routine
> column_mask_is_overflowed, column_mask_is_set and macro
> COLUMN_MASK_BIT, COLUMN_MASK_SIZE.
> We need this helpers in further refactoring.
> 
> Needed for #3571
> ---
>   src/box/column_mask.h | 39 +++++++++++++++++++++++++++++++++------
>   1 file changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/src/box/column_mask.h b/src/box/column_mask.h
> index d71911d46..6e9c0f81a 100644
> --- a/src/box/column_mask.h
> +++ b/src/box/column_mask.h
> @@ -50,7 +50,9 @@
>    * in such case we set not one bit, but a range of bits.
>    */
>   
> -#define COLUMN_MASK_FULL UINT64_MAX
> +#define COLUMN_MASK_FULL	UINT64_MAX
> +#define COLUMN_MASK_BIT(n)	(((uint64_t)1)<<(n))

1. COLUMN_MASK_BIT will return 0, when >= 64. Please, fix it
to return the last bit in such a case. Otherwise it is not
COLUMN_MASK_BIT, but just <<.

> +#define COLUMN_MASK_SIZE	((int)(sizeof(uint64_t)*8))

2. Not sure if it is worth macrosing, because anyway column
mask every where is uint64_t. If we want to hide a size,
we should introduce column_mask_t type.

>   
>   /**
>    * Set a bit in the bitmask corresponding to a
> @@ -90,10 +92,35 @@ column_mask_set_range(uint64_t *column_mask, uint32_t first_fieldno_in_range)
>   		*column_mask |= COLUMN_MASK_FULL << first_fieldno_in_range;
>   	} else {
>   		/* A range outside "short" range. */
> -		*column_mask |= ((uint64_t) 1) << 63;
> +		*column_mask |= COLUMN_MASK_BIT(COLUMN_MASK_SIZE - 1);
>   	}
>   }
>   
> +/**
> + * Test if overflow flag set in mask.
> + * @param column_mask Mask to test.
> + * @retval true If mask overflowed, false otherwise.
> + */
> +static inline bool
> +column_mask_is_overflowed(uint64_t column_mask)
> +{
> +	return column_mask & COLUMN_MASK_BIT(COLUMN_MASK_SIZE - 1);
> +}
> +
> +/**
> + * Test a bit in the bitmask corresponding to a column fieldno.
> + * @param column_mask Mask to test.
> + * @param fieldno Fieldno number to test (index base must be 0).
> + * @retval true If bit corresponding to a column fieldno.
> + * @retval false if bit is not set or fieldno > COLUMN_MASK_SIZE.
> + */
> +static inline bool
> +column_mask_fieldno_is_set(uint64_t column_mask, uint32_t fieldno)
> +{
> +	return fieldno < COLUMN_MASK_SIZE &&
> +	       (column_mask & COLUMN_MASK_BIT(fieldno)) != 0;
> +}

3. If a field no >= 63, you should return true, if the last bit of
the mask is set - this is how the last bit works here. The bit 63
means fields [63, +inf].

Also, please, write tests for the new functions. We have unit tests
for column mask in test/unit/column_mask.c.

> +
>   /**
>    * True if the update operation does not change the key.
>    * @param key_mask Key mask.
> -- 
> 2.20.1
> 
>