Re: [Tarantool-patches] [PATCH v2 06/10] region: use aligned allocations where necessary

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the review!

On 28/05/2020 22:35, Timur Safin wrote:
> 
> 
> : From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> : 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.