From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f49.google.com (mail-lf1-f49.google.com [209.85.167.49]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 0F838469719 for ; Fri, 30 Oct 2020 13:15:22 +0300 (MSK) Received: by mail-lf1-f49.google.com with SMTP id l2so7204651lfk.0 for ; Fri, 30 Oct 2020 03:15:22 -0700 (PDT) Date: Fri, 30 Oct 2020 13:15:19 +0300 From: Cyrill Gorcunov Message-ID: <20201030101519.GE198833@grain> References: <20201014133535.224573-1-gorcunov@gmail.com> <20201014133535.224573-3-gorcunov@gmail.com> <8f3da283-fc9a-31ce-cc96-e52963364a75@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8f3da283-fc9a-31ce-cc96-e52963364a75@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v8 2/4] module_cache: move module handling into own subsystem List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tml 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 > > + > > +#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