From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [94.100.177.89]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 36F1E46971A for ; Sat, 7 Dec 2019 19:35:02 +0300 (MSK) References: <8187f6bb57792948fb122d3c2cb3cfc72975fa77.1574965971.git.roman.habibov@tarantool.org> <11d5344a-0830-9c5c-9e75-30564aa257d0@tarantool.org> <3EB26BFF-D4AA-4AA0-BD3B-83992975C4BD@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Sat, 7 Dec 2019 17:34:59 +0100 MIME-Version: 1.0 In-Reply-To: <3EB26BFF-D4AA-4AA0-BD3B-83992975C4BD@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH v2 1/3] box: introduce constraint names hash table List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Roman Khabibov , tarantool-patches@dev.tarantool.org Thanks for the fixes! >>> diff --git a/src/box/constraint_def.h b/src/box/constraint_def.h >>> new file mode 100755 >>> index 000000000..ef238eb46 >>> --- /dev/null >>> +++ b/src/box/constraint_def.h >>> @@ -0,0 +1,83 @@ >>> +#ifndef INCLUDES_BOX_CONSTRAINT_DEF_H >>> +#define INCLUDES_BOX_CONSTRAINT_DEF_H >> >> 3. Please, take a look at other header guards. They >> have a certain naming policy. For example, tuple.h in >> box/ has a guard TARANTOOL_BOX_TUPLE_H_INCLUDED. I guess >> you could use TARANTOOL_BOX_CONSTRAINT_DEF_H_INCLUDED. >> >> But even better would be to use #pragma once. I don't >> know whether they are allowed in our code though. > I think #pragma is disallowed, because I didn’t found it by grep in the > src. > I asked Kirill. He said that pragma is a great idea. Please, use it in all new files, including this one. >>> --- a/src/box/space.h >>> +++ b/src/box/space.h >>> @@ -516,6 +519,42 @@ space_add_ck_constraint(struct space *space, struct ck_constraint *ck); >>> +struct constraint_def * >>> +space_constraint_def_by_name(struct space *space, const char *name); >>> + >>> +/** >>> + * Put node with @a def to the constraint hash table of @a space. >>> + * >>> + * @param space Space. >>> + * @param def Constraint def. >>> + * >>> + * @retval 0 Success. >>> + * @retval -1 Memory allocation error. >>> + */ >>> +int >>> +space_put_constraint(struct space *space, struct constraint_def *def); >>> + >>> +/** >>> + * Remove node with @a name from the constraint hash table of @a >>> + * space. But don't destroy the constraint def object binded to >>> + * this @a name. >>> + * >>> + * @param space Space. >>> + * @param name Constraint name. >>> + */ >>> +void >>> +space_drop_constraint(struct space *space, const char *name); >> >> 6. That function is a bit confusing. I understand, why >> do you need to not destroy a constraint right here. But >> drop() name really assumes that. Moreover, in most of the >> places you do space_constraint_def_by_name() before >> space_drop_constraint(). So this is a double search. I >> propose you to rename drop to pop, and make it return the >> old constraint. It will make clear, that the constraint is >> not deleted here, and will eliminate the double search. > But the first one is almost copy of the second one. Is it ok? Yes. These two functions are small enough. I tried to make a third one which they would use, but it did not make the code smaller. > diff --git a/src/box/space.h b/src/box/space.h > index 7926aa65e..0c23c224c 100644 > --- a/src/box/space.h > +++ b/src/box/space.h > @@ -234,6 +235,8 @@ struct space { > * of parent constraints as well as child ones. > */ > uint64_t fk_constraint_mask; > + /** Hash table with constraint names. */ > + struct mh_strnptr_t *constraint_names; Please, rename to 'constraints'. These are not just 'names' anymore. Sorry for the nit.