[Tarantool-patches] [PATCH v2 4/7] qsync: provide a binary form of CONFIRM/ROLLBACK entries
Cyrill Gorcunov
gorcunov at gmail.com
Fri Jul 24 21:07:52 MSK 2020
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.
More information about the Tarantool-patches
mailing list