From: Cyrill Gorcunov <gorcunov@gmail.com> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Cc: tml <tarantool-patches@dev.tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v8 2/4] module_cache: move module handling into own subsystem Date: Fri, 30 Oct 2020 13:15:19 +0300 [thread overview] Message-ID: <20201030101519.GE198833@grain> (raw) In-Reply-To: <8f3da283-fc9a-31ce-cc96-e52963364a75@tarantool.org> On Thu, Oct 29, 2020 at 11:15:54PM +0100, Vladislav Shpilevoy wrote: ... > > + > > +/** > > + * Parse function descriptor from a string. > > + */ > > +void > > +parse_func_name(const char *str, struct func_name_desc *d) > > 1. The function is not used out of this file. Please, make it > static. Yup, thanks! > > + > > +/** > > + * Find a path to a module using Lua's package.cpath. > > + * > > + * @param ctx module find context > > + * @param[out] ctx->path path to shared library. > > + * @param[out] ctx->path_len size of @a ctx->path buffer. > > 2. The last two lines are not params. The only parameter is ctx. If > you want to describe it, it should be done under the same @param. > Otherwise the comment looks mismatching the signature. I gonna say > that again. I don't force to use Doxygen, but if you decide to use it, > it should be done correctly. Well, I didnt find any reference for the correct way to describe such updates of internal structures. I think I better drop this doxy-style comment. > > > + * > > + * @return 0 on succes, -1 otherwise, diag is set. > > + */ > > +static int > > +module_find(struct module_find_ctx *ctx) > > diff --git a/src/box/module_cache.h b/src/box/module_cache.h > > new file mode 100644 > > index 000000000..3f275024a > > --- /dev/null > > +++ b/src/box/module_cache.h > > @@ -0,0 +1,156 @@ > > +/* > > + * SPDX-License-Identifier: BSD-2-Clause > > + * > > + * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file. > > + */ > > + > > +#pragma once > > + > > +#include <small/rlist.h> > > + > > +#include "func_def.h" > > 3. Func_def heavily depends on schema. Also it is a part of > all the _func handling, meaning this is a cyclic dependency now - > module_cache depends on func and vice versa. Nope. Where you get it? func_def.h is not touched in this series at all. What we have now module_cache.h: func_def.h func.h: module_cache.h func_def.h func_def.h: trivia/util.h field_def.h opt_def.h I don't understand which cyclic dependency you mean. > > + > > +/*** > > + * Parse function name into a name descriptor. > > + * > > 4. Trailing whitespace. +1 > > +/** > > + * Load a new module symbol. > > + * > > + * @param mod_sym symbol to load. > > + * > > + * @returns 0 on succse, -1 otherwise, diag is set. > > + */ > > 5. I see you added comments for each function twice - in its > declaration and implementation. Please, don't. Such double > comments usually quicky get desynchronized. Even now they are > not the same. We always comment functions on their declaration, > if it is separated from implementation. > > The same for the other commits and other places in this commit. OK
next prev parent reply other threads:[~2020-10-30 10:15 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-14 13:35 [Tarantool-patches] [PATCH v8 0/4] box/cbox: implement cfunc Lua module Cyrill Gorcunov 2020-10-14 13:35 ` [Tarantool-patches] [PATCH v8 1/4] box/func: factor out c function entry structure Cyrill Gorcunov 2020-10-29 22:15 ` Vladislav Shpilevoy 2020-10-30 9:51 ` Cyrill Gorcunov 2020-10-31 0:13 ` Vladislav Shpilevoy 2020-10-31 15:27 ` Cyrill Gorcunov 2020-10-14 13:35 ` [Tarantool-patches] [PATCH v8 2/4] module_cache: move module handling into own subsystem Cyrill Gorcunov 2020-10-29 22:15 ` Vladislav Shpilevoy 2020-10-30 10:15 ` Cyrill Gorcunov [this message] 2020-10-31 0:15 ` Vladislav Shpilevoy 2020-10-31 15:29 ` Cyrill Gorcunov 2020-10-14 13:35 ` [Tarantool-patches] [PATCH v8 3/4] box/cbox: implement cbox Lua module Cyrill Gorcunov 2020-10-29 22:15 ` Vladislav Shpilevoy 2020-10-30 12:51 ` Cyrill Gorcunov 2020-10-31 0:21 ` Vladislav Shpilevoy 2020-10-31 21:59 ` Cyrill Gorcunov 2020-11-01 8:26 ` Cyrill Gorcunov 2020-11-02 22:25 ` Vladislav Shpilevoy 2020-11-03 7:26 ` Cyrill Gorcunov 2020-11-12 22:54 ` Vladislav Shpilevoy 2020-11-13 18:30 ` Cyrill Gorcunov 2020-10-14 13:35 ` [Tarantool-patches] [PATCH v8 4/4] test: box/cfunc -- add simple module test Cyrill Gorcunov
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=20201030101519.GE198833@grain \ --to=gorcunov@gmail.com \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v8 2/4] module_cache: move module handling into own subsystem' \ /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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox