Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH] lua: keeping the pointer type in msgpackffi.decode

["Maria Khaydich" <maria.khaydich@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 3414 bytes --]


Fixed the commit message according to your comments. 
 
>Понедельник, 9 декабря 2019, 2:36 +03:00 от Alexander Turenko < alexander.turenko@tarantool.org >:
>  
>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 < alexander.turenko@tarantool.org >:
>
>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<unsigned char *>, but now it is cdata<char *> or cdata<const char
>  | *> 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()
>  | | <write msgpack data to 'buf'>
>  | | 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
>  
 
 
--
Maria Khaydich
 
 

[-- Attachment #2: Type: text/html, Size: 5269 bytes --]