From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (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 DB07E469710 for ; Fri, 29 May 2020 02:07:04 +0300 (MSK) References: <31172635747a4f2d20c37525379285aa5ba69ab2.1590622225.git.v.shpilevoy@tarantool.org> <049e01d6352f$96a7e950$c3f7bbf0$@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Fri, 29 May 2020 01:07:02 +0200 MIME-Version: 1.0 In-Reply-To: <049e01d6352f$96a7e950$c3f7bbf0$@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Timur Safin , tarantool-patches@dev.tarantool.org, alyapunov@tarantool.org, korablev@tarantool.org Thanks for the review! On 28/05/2020 22:35, Timur Safin wrote: > > > : 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? To avoid bugs like this: https://github.com/tarantool/tarantool/commit/d1647590ec4263592d6408cd4c16c2c2d55a7847 Talking of simpler/clearer - I consider my way both simpler and clearer. And safer. When type name is written only once, in variable declaration, there is no chance in screwing it later, if you use sizeof(variable) and typeof(variable) instead of duplicating type name. > : @@ -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 :) No. I explained it above. > 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) We can't work without typeof(), because our generic data structures heavily depend on it: rlist, stailq, heap. Tarantool uses them so extensively, that it probably would be easier to implement them from the scratch somehow, than get rid of them.