From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id AF7A228546 for ; Fri, 15 Feb 2019 12:05:08 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ZL6uX_EofY_r for ; Fri, 15 Feb 2019 12:05:08 -0500 (EST) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id E9C2628540 for ; Fri, 15 Feb 2019 12:05:07 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v1 1/4] box: introduce new helpers in column_mask.h References: From: Vladislav Shpilevoy Message-ID: <2c60dc5e-f5cb-001e-b81f-a7e04967e1dd@tarantool.org> Date: Fri, 15 Feb 2019 20:05:05 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, Kirill Shcherbatov 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 > >