Re: [Tarantool-patches] [PATCH 1/4] lua: move serializer helpers into its own file

[Alexander Turenko via Tarantool-patches <tarantool-patches@dev.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.