From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 3DFCF4765E0 for ; Sat, 26 Dec 2020 00:42:51 +0300 (MSK) Date: Sat, 26 Dec 2020 00:42:45 +0300 From: Igor Munkin Message-ID: <20201225214245.GG5396@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH luajit v2 1/7] utils: introduce leb128 reader and writer List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Kaplun Cc: tarantool-patches@dev.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 > 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 @@ > +/* > +** Reads a value from a buffer of bytes to a int64_t output. Typo: s/a int64_t/an int64_t/g. flag means the note relates to all comments below. > +** No bounds checks for the buffer. Returns number of bytes read. > +*/ > +/* > +** 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. > +*/ > 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 @@ > +#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. > + > -- > 2.28.0 > -- Best regards, IM