Re: [Tarantool-patches] [PATCH v2 4/7] qsync: provide a binary form of CONFIRM/ROLLBACK entries

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Fri, Jul 24, 2020 at 12:10:52AM +0200, Vladislav Shpilevoy wrote:
> >  
> > +/** CONFIRM/ROLLBACK entries encoded in MsgPack. */
> > +struct PACKED request_synchro_body {
> 
> 1. Please, call it _bin, not _body. Look at the examples iproto_header_bin,
> iproto_body_bin. _body_bin also would be fine. But should end with _bin
> anyway.

Actually in thi file we already have

struct PACKED request_replace_body {
	uint8_t m_body;
	uint8_t k_space_id;
	uint8_t m_space_id;
	uint32_t v_space_id;
	uint8_t k_tuple;
};

that's why I named it this way. But sure renamed to
have _bin postfix.

> > @@ -897,26 +897,19 @@ static int
> >  xrow_encode_confirm_rollback(struct xrow_header *row, struct region *region,
> >  			     uint32_t replica_id, int64_t lsn, int type)
> >  {
> > -	size_t len = mp_sizeof_map(2) + mp_sizeof_uint(IPROTO_REPLICA_ID) +
> > -		     mp_sizeof_uint(replica_id) + mp_sizeof_uint(IPROTO_LSN) +
> > -		     mp_sizeof_uint(lsn);
> > -	char *buf = (char *)region_alloc(region, len);
> > -	if (buf == NULL) {
> > -		diag_set(OutOfMemory, len, "region_alloc", "buf");
> > +	struct request_synchro_body *body;
> > +
> > +	body = region_alloc(region, sizeof(*body));
> 
> 2. I see you omit explicit type casts. And use (void *) in the next commits,
> when need to cast between 2 non-void types. Both ways are not used in Tarantool.
> At least they are not part of the code style. Personally I am fine if we will
> adopt your way. It is not too conflicting with the old code (don't need to
> change the old code then), is more compact. But you need to raise that question
> with other teammates. So as we could write down that way in the doc, if everyone
> agrees. And force it in the newer commits.

I tried to ask, which end up in a flame war. I gave up. I think the
region_alloc is the same as malloc and etc and we already have a
number of places where we use implicit casting (grep region_alloc).
So I think we can use both approaches, while I prefer a short from
which is allowed by C standart.