<HTML><BODY><div class="js-helper js-readmsg-msg">
<style type="text/css">
</style>
<div>
<div id="style_15760626471736597026_BODY">
<div class="class_1576081089">
<div>Fixed the commit message according to your comments. </div>
<div> </div>
<div class="mail-quote-collapse">
<blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><span data-email="alexander.turenko@tarantool.org" data-name="Alexander Turenko" data-quote-id="1692284422581715304" data-timestamp="1575848160" data-type="sender">Понедельник, 9 декабря 2019, 2:36 +03:00 от Alexander Turenko <<a href="/compose?To=alexander.turenko@tarantool.org">alexander.turenko@tarantool.org</a>>:<br>
</span>
<div data-quote-id="1692284422581715304" data-type="body">
<div id="">
<div class="js-helper_mailru_css_attribute_postfix js-readmsg-msg_mailru_css_attribute_postfix">
<style type="text/css">
</style>
<div>
<div id="style_15758482081113592859_BODY_mailru_css_attribute_postfix">LGTM for the code.<br>
<br>
Requested changes for the commit message, see below.<br>
<br>
Please, fix it and proceed to the next reviewer (I suggest to send the<br>
patch to Vlad). Leave me in CC, please.<br>
<br>
WBR, Alexander Turenko.<br>
<br>
On Wed, Nov 13, 2019 at 02:21:06PM +0300, Maria Khaydich wrote:<br>
><br>
><br>
> Thank you for the review. Proposed changes look reasonable to me so<br>
> I’ve squashed your FIXUP commits and also added follow up patch as a<br>
> separate commit. <br>
> <br>
> >Среда, 6 ноября 2019, 3:44 +03:00 от Alexander Turenko <<a rel="noopener noreferrer">alexander.turenko@tarantool.org</a>>:<br>
<br>
I take a glance at commits in [1]. Sorry for the big delay.<br>
<br>
The message of the first commit ('msgpackffi.decode can now be assigned<br>
to buf.rpos') now reflects all intermediate changes. It should not:<br>
please, reword the commit messsage to reflect the resulting state of the<br>
patch.<br>
<br>
In this particular case there is nothing in messages I left in fixup<br>
commits that is worth to save in the resulting commit message.<br>
<br>
The original commit message is the following, I'll comment it:<br>
<br>
> msgpackffi.decode can now be assigned to buf.rpos<br>
<br>
We usually prefix commit messages with a subsystem that is the subject<br>
of a commit: 'lua' in this case.<br>
<br>
I would state what was changed in the header (msgpackffi.decode*()<br>
return value type) and then describe why (to assign to rpos) in the<br>
commit message.<br>
<br>
Maybe this is just my personal taste. I don't insist anyway.<br>
<br>
><br>
> Function decode of module msgpackffi was passing<br>
> value of type const unsigned char * to a C function<br>
> that accepts arguments of type const char *.<br>
<br>
msgpackffi.decode() returns 'unsigned char *', then a user passes this<br>
result to a function that accepts 'const char *'? Is I interpret it<br>
right? Didn't get this message, to be honest.<br>
<br>
><br>
> Closes #3926<br>
<br>
I would reword it like so (just for example, let's use any wording you<br>
comfortable with):<br>
<br>
| lua: don't modify pointer type in msgpackffi.decode*<br>
|<br>
| When msgpackffi.decode() and msgpackffi.decode_unchecked() are called<br>
| with a buffer (cdata<[const] char *>) parameter, they return two values:<br>
| a decoded value and a new position within the buffer (it is a pointer<br>
| too).<br>
|<br>
| This commit changes a type of the returned pointer: it was<br>
| cdata<unsigned char *>, but now it is cdata<char *> or cdata<const char<br>
| *> depending of a function parameter.<br>
|<br>
| The primary reason why this change was made is to use<br>
| msgpackffi.decode*() with 'buffer' module seamlessly: read msgpack from<br>
| 'rpos' field and then promote 'rpos' to the new position:<br>
|<br>
| | local msgpackffi = require('msgpackffi')<br>
| | local buffer = require('buffer')<br>
| |<br>
| | local buf = buffer.ibuf()<br>
| | <write msgpack data to 'buf'><br>
| | local res<br>
| | res, buf.rpos = msgpackffi.decode(buf.rpos, buf:size())<br>
|<br>
| Before this commit the latter statement fails with the following error:<br>
|<br>
| > cannot convert 'const unsigned char *' to 'char *'<br>
|<br>
| Closes #3926<br>
<br>
[1]: <a href="https://github.com/tarantool/tarantool/commits/eljashm/gh-3926-msgpackffi.decode-assignment-to-buf.rpos" rel="noopener noreferrer" target="_blank">https://github.com/tarantool/tarantool/commits/eljashm/gh-3926-msgpackffi.decode-assignment-to-buf.rpos</a><br>
</div>
</div>
</div>
</div>
</div>
</blockquote>
</div>
<div> </div>
<div data-signature-widget="container">
<div data-signature-widget="content">
<div>--<br>
Maria Khaydich</div>
</div>
</div>
<div> </div>
</div>
</div>
</div>
</div>
<div> </div>
</BODY></HTML>