From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [94.100.177.92]) (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 5BDA9469719 for ; Fri, 16 Oct 2020 01:27:10 +0300 (MSK) From: "Timur Safin" References: <23f2b9468c529a6276ac75c1cfe81e60e1fabfb2.1602629628.git.tsafin@tarantool.org> In-Reply-To: Date: Fri, 16 Oct 2020 01:27:08 +0300 Message-ID: <055301d6a342$51ed3f00$f5c7bd00$@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Content-Language: ru Subject: Re: [Tarantool-patches] [PATCH 2.X v4 4/4] module api: box_ibuf_* wrappers List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: 'Vladislav Shpilevoy' , alexander.turenko@tarantool.org Cc: tarantool-patches@dev.tarantool.org : From: Vladislav Shpilevoy : Subject: Re: [Tarantool-patches] [PATCH 2.X v4 4/4] module api: box_ibuf_* : wrappers : : Thanks for the patch! : : See 4 comments below. : ... : > + : > +#include : > + : > +#include "ibuf.h" : > +#include : : 1. Please, don't include non-system headers using <>. Use "". : The same for box/ibuf.h. Ok. : : > + : > +void * : > +box_ibuf_reserve(box_ibuf_t *ibuf, size_t size) : > +{ : > + return ibuf_reserve(ibuf, size); : : 2. It should set a diag error in case of fail. This is already updated accordingly in the branch tsafin/gh-5273-expand-module-api-v4. : : > +} : > + : > +void : > +box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos) : > +{ : > + assert(ibuf != NULL); : > + if (ibuf == NULL) : > + return; : > + if (rpos != NULL) : > + *rpos = &ibuf->rpos; : > + if (wpos != NULL) : > + *wpos = &ibuf->wpos; : : 3. I would better assume all the arguments are not NULL, and document : it. Especially ibuf itself. We need some border where to stop the : sanity checks, and this looks like an overkill already. For example, : box_tuple_ref() also works with a pointer, but it does not check for : it being not NULL. It is just stupid to call method of an object with : that object passed as NULL. The same below. Please see updated branch tsafin/gh-5273-expand-module-api-v4 for the current version. For rpos and wpos I need to check for NULL because we may call for only single, particular argument (like we do in some cases in current external module) thus we pass NULL there. : : > +} : > + : > +void : > +box_ibuf_write_range(box_ibuf_t *ibuf, char ***wpos, char ***end) : > +{ : > + if (ibuf == NULL) : > + return; : > + if (wpos != NULL) : > + *wpos = &ibuf->wpos; : > + if (end != NULL) : > + *end = &ibuf->end; : > + : > +} ... : > + : > +/** : > + * Reserve requested amount of bytes in ibuf buffer : : 4. Please, finish sentences with a dot. I don't understand. : Is it so hard for people to put a dot in the end? Does it : cost money or something? Do people write the same way in : other places, like emails, documents? Why is the code allowed : to be treated worse? : : Talking of the comments content - they are mostly useless. : What is read range, what is write range? How do they relate? : Is a user supposed to update them manually, when work with the : buffer? Current description makes it impossible to use : box_ibuf_t without reading the source code. What users are : not supposed to do usually to be able to use the API. I'll return to comments and test in a follow-up patchset (see my earlier discussion with A.Turenko) and there I'll put all dots. Promise! In very meaningful comments. Promise!! Timur