Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: Cyrill Gorcunov <gorcunov@tarantool.org>,
	tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 1/4] lua: move serializer helpers into its own file
Date: Mon, 5 Jul 2021 09:30:37 +0300
Message-ID: <20210705063037.n5p7mvqyyg4bxi7a@tkn_work_nb> (raw)
In-Reply-To: <98109d2e-4efd-1ee5-86e2-b2d1574653fa@tarantool.org>

On Sun, Jul 04, 2021 at 03:10:00PM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 3 comments below.
> 
> > diff --git a/src/lua/init.c b/src/lua/init.c
> > index 93e93a103..ff11d202b 100644
> > --- a/src/lua/init.c
> > +++ b/src/lua/init.c
> > @@ -69,6 +69,10 @@
> >  #include <readline/readline.h>
> >  #include <readline/history.h>
> >  
> > +/* Don't include the entire header only for *_init(). */
> 
> 1. But what is the problem with that? It would be simpler and
> consistent with the other inits.

It looks more accurate for me: acquire only what we actually use. Just
like defining an opacue structure in C or using `from foo import bar` in
Python.

It is not the main reason for me, but it also can positively impact
compilation time (less preprocessing).

However, maybe, it would be more accurate to define a macro like
LUA_MODULE_INIT_ONLY before including those headers and add #if
conditions to them.

Anyway, since it looks unusual for you, I'll stick with just #include
<lua/serializer.h>.

> > diff --git a/src/lua/serializer.c b/src/lua/serializer.c
> > new file mode 100644
> > index 000000000..6c3dd73af
> > --- /dev/null
> > +++ b/src/lua/serializer.c
> > @@ -0,0 +1,651 @@
> > <..stripped license text..>
> > +#include <assert.h>
> > +#include <stdbool.h>
> > +#include <math.h> /* modf, isfinite */
> > +#include <lua.h>
> > +#include <lauxlib.h>
> 
> 2. These are already included in the header. And some of the others
> below too.

I discussed it once (a long time ago) with Vladimir D. He said that the
rule of thumb is to include a header into a foo.c file if we use
something from it: disregarding whether the header is included into
foo.h or not.

It looks meaningful for me: declare where use. I guess it'll ease future
changes of only .c or only .h a bit.

I'll leave it as is if you don't object.

To be honest, #include directives management is a pain. If there is some
good linter, I would integrate it into the project.

> > diff --git a/src/lua/serializer.h b/src/lua/serializer.h
> > new file mode 100644
> > index 000000000..54b0bc11a
> > --- /dev/null
> > +++ b/src/lua/serializer.h
> > @@ -0,0 +1,344 @@
> > +#ifndef TARANTOOL_LUA_SERIALIZER_H_INCLUDED
> > +#define TARANTOOL_LUA_SERIALIZER_H_INCLUDED
> 
> 3. According to the code style, new files should use `#pragma once`.

Changed.

WBR, Alexander Turenko.

  reply	other threads:[~2021-07-05  6:31 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-23 19:12 [Tarantool-patches] [PATCH 0/4] RFC: Isolate serializer helpers Alexander Turenko via Tarantool-patches
2021-06-23 19:12 ` [Tarantool-patches] [PATCH 1/4] lua: move serializer helpers into its own file Alexander Turenko via Tarantool-patches
2021-07-04 13:10   ` Vladislav Shpilevoy via Tarantool-patches
2021-07-05  6:30     ` Alexander Turenko via Tarantool-patches [this message]
2021-07-05 20:59       ` Vladislav Shpilevoy via Tarantool-patches
2021-06-23 19:12 ` [Tarantool-patches] [PATCH 2/4] lua: move luaL_newserializer() comment into header Alexander Turenko via Tarantool-patches
2021-06-23 19:12 ` [Tarantool-patches] [PATCH 3/4] lua: split serializer functions into sections Alexander Turenko via Tarantool-patches
2021-06-23 19:12 ` [Tarantool-patches] [PATCH 4/4] test: add a basic unit test for serializer helpers Alexander Turenko via Tarantool-patches
2021-06-24  6:17 ` [Tarantool-patches] [PATCH 0/4] RFC: Isolate " Cyrill Gorcunov via Tarantool-patches
2021-06-28  6:31 ` Cyrill Gorcunov via Tarantool-patches
2021-07-04 13:09 ` Vladislav Shpilevoy via Tarantool-patches
2021-07-05  6:30   ` Alexander Turenko via Tarantool-patches
2021-07-07 10:08 ` Alexander Turenko via Tarantool-patches
2021-07-07 19:09   ` Alexander Turenko via Tarantool-patches
2021-07-07 22:16     ` Vladislav Shpilevoy via Tarantool-patches
2021-07-12  7:51       ` Alexander Turenko via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210705063037.n5p7mvqyyg4bxi7a@tkn_work_nb \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=alexander.turenko@tarantool.org \
    --cc=gorcunov@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git