From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (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 57D8346971A for ; Mon, 9 Dec 2019 02:36:37 +0300 (MSK) Date: Mon, 9 Dec 2019 02:36:32 +0300 From: Alexander Turenko Message-ID: <20191208233631.spmeybcqphadf3pr@tkn_work_nb> References: <20190912174448.25680-1-maria.khaydich@tarantool.org> <20191106004408.ajxf53urlqzy37ce@tkn_work_nb> <1573644066.297818688@f177.i.mail.ru> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1573644066.297818688@f177.i.mail.ru> Subject: Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH] msgpackffi.decode can now be assigned to buf.rpos List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Maria Khaydich Cc: tarantool-patches@freelists.org, tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy LGTM for the code. Requested changes for the commit message, see below. Please, fix it and proceed to the next reviewer (I suggest to send the patch to Vlad). Leave me in CC, please. WBR, Alexander Turenko. On Wed, Nov 13, 2019 at 02:21:06PM +0300, Maria Khaydich wrote: > > > Thank you for the review. Proposed changes look reasonable to me so > I’ve squashed your FIXUP commits and also added follow up patch as a > separate commit.  >   > >Среда, 6 ноября 2019, 3:44 +03:00 от Alexander Turenko : I take a glance at commits in [1]. Sorry for the big delay. The message of the first commit ('msgpackffi.decode can now be assigned to buf.rpos') now reflects all intermediate changes. It should not: please, reword the commit messsage to reflect the resulting state of the patch. In this particular case there is nothing in messages I left in fixup commits that is worth to save in the resulting commit message. The original commit message is the following, I'll comment it: > msgpackffi.decode can now be assigned to buf.rpos We usually prefix commit messages with a subsystem that is the subject of a commit: 'lua' in this case. I would state what was changed in the header (msgpackffi.decode*() return value type) and then describe why (to assign to rpos) in the commit message. Maybe this is just my personal taste. I don't insist anyway. > > Function decode of module msgpackffi was passing > value of type const unsigned char * to a C function > that accepts arguments of type const char *. msgpackffi.decode() returns 'unsigned char *', then a user passes this result to a function that accepts 'const char *'? Is I interpret it right? Didn't get this message, to be honest. > > Closes #3926 I would reword it like so (just for example, let's use any wording you comfortable with): | lua: don't modify pointer type in msgpackffi.decode* | | When msgpackffi.decode() and msgpackffi.decode_unchecked() are called | with a buffer (cdata<[const] char *>) parameter, they return two values: | a decoded value and a new position within the buffer (it is a pointer | too). | | This commit changes a type of the returned pointer: it was | cdata, but now it is cdata or cdata depending of a function parameter. | | The primary reason why this change was made is to use | msgpackffi.decode*() with 'buffer' module seamlessly: read msgpack from | 'rpos' field and then promote 'rpos' to the new position: | | | local msgpackffi = require('msgpackffi') | | local buffer = require('buffer') | | | | local buf = buffer.ibuf() | | | | local res | | res, buf.rpos = msgpackffi.decode(buf.rpos, buf:size()) | | Before this commit the latter statement fails with the following error: | | > cannot convert 'const unsigned char *' to 'char *' | | Closes #3926 [1]: https://github.com/tarantool/tarantool/commits/eljashm/gh-3926-msgpackffi.decode-assignment-to-buf.rpos