Re: [Tarantool-patches] [PATCH luajit v2 1/7] utils: introduce leb128 reader and writer

[Igor Munkin <imun@tarantool.org>]

Sergey,

Thanks for the patch! LGTM, except the several nits below.

On 25.12.20, Sergey Kaplun wrote:
> Most of the numeric data written by the memory profiler is encoded
> via LEB128 compression. This patch introduces the module for encoding
> and decoding 64bit number to LEB128 form.
> 
> Part of tarantool/tarantool#5442
> ---
> 
> Changes in v2:
>   - Removed reader funciton's parameter named guard.
>   - Code style fixes.
> 
>  src/Makefile          |   3 +-
>  src/Makefile.dep      |   7 ++-
>  src/lj_utils.h        |  58 +++++++++++++++++++
>  src/lj_utils_leb128.c | 132 ++++++++++++++++++++++++++++++++++++++++++
>  src/ljamalg.c         |   1 +
>  5 files changed, 197 insertions(+), 4 deletions(-)
>  create mode 100644 src/lj_utils.h
>  create mode 100644 src/lj_utils_leb128.c
> 
> diff --git a/src/Makefile b/src/Makefile
> index 2786348..dc2ddb6 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -466,6 +466,7 @@ endif
>  DASM_FLAGS= $(DASM_XFLAGS) $(DASM_AFLAGS)
>  DASM_DASC= vm_$(DASM_ARCH).dasc
>  
> +UTILS_O= lj_utils_leb128.o

Minor: I personally believe this is excess and you can simply move this
object file to LJCORE_O list. BUILDVM_O is built with another toolchain;
LJLIB_O is used for generating auxiliary headers with buildvm.
Everything else is mentioned in LJCORE_O.

>  BUILDVM_O= host/buildvm.o host/buildvm_asm.o host/buildvm_peobj.o \
>  	   host/buildvm_lib.o host/buildvm_fold.o
>  BUILDVM_T= host/buildvm

<snipped>

> diff --git a/src/lj_utils.h b/src/lj_utils.h
> new file mode 100644
> index 0000000..1671e8e
> --- /dev/null
> +++ b/src/lj_utils.h
> @@ -0,0 +1,58 @@

<snipped>

> +/*
> +** Reads a value from a buffer of bytes to a int64_t output.

Typo: s/a int64_t/an int64_t/g.

<g> flag means the note relates to all comments below.

> +** No bounds checks for the buffer. Returns number of bytes read.
> +*/

<snipped>

> +/*
> +** Writes a value from an signed 64-bit input to a buffer of bytes.

Typo: s/an signed/a signed/.

> +** No bounds checks for the buffer. Returns number of bytes written.
> +*/

<snipped>

> diff --git a/src/lj_utils_leb128.c b/src/lj_utils_leb128.c
> new file mode 100644
> index 0000000..ce8081b
> --- /dev/null
> +++ b/src/lj_utils_leb128.c
> @@ -0,0 +1,132 @@

<snipped>

> +#define LINK_BIT               (0x80)
> +#define MIN_TWOBYTE_VALUE      (0x80)
> +#define PAYLOAD_MASK           (0x7f)
> +#define SHIFT_STEP             (7)
> +#define LEB_SIGN_BIT           (0x40)

Typo: Why did you change the whitespace here? Everything was OK with it
in the previous version.

> +

<snipped>

> -- 
> 2.28.0
> 

-- 
Best regards,
IM