[tarantool-patches] Re: [PATCH v1 1/4] box: introduce new helpers in column_mask.h
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Feb 15 20:05:05 MSK 2019
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
>
>
More information about the Tarantool-patches
mailing list