From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (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 35B34469710 for ; Thu, 28 May 2020 23:35:59 +0300 (MSK) From: "Timur Safin" References: <31172635747a4f2d20c37525379285aa5ba69ab2.1590622225.git.v.shpilevoy@tarantool.org> In-Reply-To: <31172635747a4f2d20c37525379285aa5ba69ab2.1590622225.git.v.shpilevoy@tarantool.org> Date: Thu, 28 May 2020 23:35:55 +0300 Message-ID: <049e01d6352f$96a7e950$c3f7bbf0$@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Content-Language: ru Subject: Re: [Tarantool-patches] [PATCH v2 06/10] region: use aligned allocations where necessary List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: 'Vladislav Shpilevoy' , tarantool-patches@dev.tarantool.org, alyapunov@tarantool.org, korablev@tarantool.org : From: Vladislav Shpilevoy : Subject: [PATCH v2 06/10] region: use aligned allocations where necessary : : diff --git a/src/box/alter.cc b/src/box/alter.cc : index dbbbcbc44..bb4254878 100644 : --- a/src/box/alter.cc : +++ b/src/box/alter.cc : @@ -537,11 +537,13 @@ space_format_decode(const char *data, uint32_t : *out_count, : *fields = NULL; : return 0; : } : - size_t size = count * sizeof(struct field_def); : + size_t size; : struct field_def *region_defs = : - (struct field_def *) region_alloc(region, size); : + region_alloc_array(region, typeof(region_defs[0]), count, : + &size); I'd say that `typeof(region_defs[0])` is very indirect way to write `struct field_def` :) Why you've chosen this way (and not simpler/clearer) way? : @@ -2452,10 +2454,12 @@ on_replace_dd_space(struct trigger * /* trigger : */, void *event) : * comparators must be updated. : */ : struct key_def **keys; : - size_t bsize = old_space->index_count * sizeof(keys[0]); : - keys = (struct key_def **) region_alloc(&fiber()->gc, bsize); : + size_t bsize; : + keys = region_alloc_array(&fiber()->gc, typeof(keys[0]), : + old_space->index_count, &bsize); Oh, now I see - for copy-paste sake :) (I still think though the explicit `struct key_def*` would be clearer for the stranger passing by.) The same question is applying multiple times below in the patch. ... ... And joking aside, putting aside all debatable complains about simpler/clearer code which might be used (which you could ignore in general), here is real complain - this is gcc preprocessor extension, which might be not working under other C99/C11-compliant compilers. Which we would rather avoid in a longer run (if standard C compatible code would not require much efforts from us, like in these cases) Regards, Timur