Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH] msgpackffi.decode can now be assigned to buf.rpos

[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