From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f66.google.com (mail-lf1-f66.google.com [209.85.167.66]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 92189445320 for ; Fri, 24 Jul 2020 21:07:55 +0300 (MSK) Received: by mail-lf1-f66.google.com with SMTP id i80so5642171lfi.13 for ; Fri, 24 Jul 2020 11:07:55 -0700 (PDT) Date: Fri, 24 Jul 2020 21:07:52 +0300 From: Cyrill Gorcunov Message-ID: <20200724180752.GE60766@grain> References: <20200723122942.196011-1-gorcunov@gmail.com> <20200723122942.196011-5-gorcunov@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v2 4/7] qsync: provide a binary form of CONFIRM/ROLLBACK entries List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tml 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.