From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id D79DD2D601 for ; Mon, 19 Nov 2018 12:58:04 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id XYmsPz9hyU8i for ; Mon, 19 Nov 2018 12:58:04 -0500 (EST) 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 turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 950452D4A5 for ; Mon, 19 Nov 2018 12:58:04 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v1 04/10] sql: create interface vstream References: From: Vladislav Shpilevoy Message-ID: <01659811-194e-e4f8-2ffc-7cd32847e6d6@tarantool.org> Date: Mon, 19 Nov 2018 20:58:02 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: imeevma@tarantool.org, tarantool-patches@freelists.org Thanks for the patch! See 4 comments below. On 17/11/2018 17:03, imeevma@tarantool.org wrote: > If we want to use functions from execute.h not only in IPROTO we > should create special interface. This interface will allow us to > create different implementations for mpstream and lua_State and > use functions from execute.c without changing them. This patch > creates such interface and its implementation for mpstream and > replaces mpstream functions in execute.c by methods of this > interface. > > Needed for #3505 > --- > src/box/CMakeLists.txt | 1 + > src/box/execute.c | 92 +++++++++-------------- > src/box/execute.h | 4 +- > src/box/iproto.cc | 13 +++- > src/box/vstream.c | 200 +++++++++++++++++++++++++++++++++++++++++++++++++ > src/box/vstream.h | 191 ++++++++++++++++++++++++++++++++++++++++++++++ > 6 files changed, 439 insertions(+), 62 deletions(-) > create mode 100644 src/box/vstream.c > create mode 100644 src/box/vstream.h > > diff --git a/src/box/vstream.c b/src/box/vstream.c > new file mode 100644 > index 0000000..d43c352 > --- /dev/null > +++ b/src/box/vstream.c > @@ -0,0 +1,200 @@ > +/* > + * Copyright 2010-2018, Tarantool AUTHORS, please see AUTHORS file. > + * > + * Redistribution and use in source and binary forms, with or > + * without modification, are permitted provided that the following > + * conditions are met: > + * > + * 1. Redistributions of source code must retain the above > + * copyright notice, this list of conditions and the > + * following disclaimer. > + * > + * 2. Redistributions in binary form must reproduce the above > + * copyright notice, this list of conditions and the following > + * disclaimer in the documentation and/or other materials > + * provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY AUTHORS ``AS IS'' AND > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED > + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL > + * AUTHORS OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, > + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR > + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF > + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF > + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > + * SUCH DAMAGE. > + */ > + > +#include "vstream.h" > +#include "mpstream.h" > +#include "iproto_constants.h" > +#include "port.h" > +#include "xrow.h" 1. The whole this file should be merged into mpstream.c. Vstream is an 'abstract' class, it even has no vstream.c file. Only header. > + > +const struct vstream_vtab mp_vstream_vtab = { > + /** encode_array = */ mp_vstream_encode_array, > + /** encode_map = */ mp_vstream_encode_map, > + /** encode_uint = */ mp_vstream_encode_uint, > + /** encode_int = */ mp_vstream_encode_int, > + /** encode_float = */ mp_vstream_encode_float, > + /** encode_double = */ mp_vstream_encode_double, > + /** encode_strn = */ mp_vstream_encode_strn, > + /** encode_nil = */ mp_vstream_encode_nil, > + /** encode_bool = */ mp_vstream_encode_bool, > + /** encode_enum = */ mp_vstream_encode_enum, > + /** encode_port = */ mp_vstream_encode_port, > + /** encode_reply_array = */ mp_vstream_encode_reply_array, > + /** encode_reply_map = */ mp_vstream_encode_reply_map, > + /** encode_array_commit = */ mp_vstream_encode_array_commit, > + /** encode_reply_commit = */ mp_vstream_encode_reply_commit, > + /** encode_map_commit = */ mp_vstream_encode_map_commit, 2. Once you did it, please, do not wrap each mpstream function with a one line wrapper which differs in first argument pointer type only. Most of this functions you can assign with an explicit cast. > +}; > + > +void > +mp_vstream_init(struct vstream *vstream, struct mpstream *mpstream) > +{ > + vstream->vtab = &mp_vstream_vtab; > + vstream->mpstream = mpstream; > + vstream->is_flatten = false; > +} > diff --git a/src/box/vstream.h b/src/box/vstream.h > new file mode 100644 > index 0000000..42f9813 > --- /dev/null > +++ b/src/box/vstream.h > @@ -0,0 +1,191 @@ > +#ifndef TARANTOOL_VSTREAM_H_INCLUDED > +#define TARANTOOL_VSTREAM_H_INCLUDED > +/* > + * Copyright 2010-2018, Tarantool AUTHORS, please see AUTHORS file. > + * > + * Redistribution and use in source and binary forms, with or > + * without modification, are permitted provided that the following > + * conditions are met: > + * > + * 1. Redistributions of source code must retain the above > + * copyright notice, this list of conditions and the > + * following disclaimer. > + * > + * 2. Redistributions in binary form must reproduce the above > + * copyright notice, this list of conditions and the following > + * disclaimer in the documentation and/or other materials > + * provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY AUTHORS ``AS IS'' AND > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED > + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL > + * AUTHORS OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, > + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR > + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF > + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF > + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > + * SUCH DAMAGE. > + */ > + > +#include "diag.h" > + > +#if defined(__cplusplus) > +extern "C" { > +#endif /* defined(__cplusplus) */ > + > +struct vstream; > +struct mpstream; > +struct lua_State; > +struct port; > + > +struct vstream_vtab { > + void (*encode_array)(struct vstream *stream, uint32_t size); > + void (*encode_map)(struct vstream *stream, uint32_t size); > + void (*encode_uint)(struct vstream *stream, uint64_t num); > + void (*encode_int)(struct vstream *stream, int64_t num); > + void (*encode_float)(struct vstream *stream, float num); > + void (*encode_double)(struct vstream *stream, double num); > + void (*encode_strn)(struct vstream *stream, const char *str, > + uint32_t len); > + void (*encode_nil)(struct vstream *stream); > + void (*encode_bool)(struct vstream *stream, bool val); > + void (*encode_enum)(struct vstream *stream, int64_t num, > + const char *str); > + int (*encode_port)(struct vstream *stream, struct port *port); > + int (*encode_reply_array)(struct vstream *stream, uint32_t size, > + uint8_t key, const char *str); > + int (*encode_reply_map)(struct vstream *stream, uint32_t size, > + uint8_t key, const char *str); > + void (*encode_array_commit)(struct vstream *stream, uint32_t id); > + void (*encode_reply_commit)(struct vstream *stream); > + void (*encode_map_commit)(struct vstream *stream); 3. encode_reply_* functions should not exist. As I said earlier, you can inline them as already existing encode_* functions. > +}; > + > +struct vstream { > + /** Virtual function table. */ > + const struct vstream_vtab *vtab; > + /** TODO: Write comment. */ > + union { > + struct mpstream *mpstream; > + struct lua_State *L; > + }; > + /** TODO: Write comment. */ > + bool is_flatten; 4. This flag should not be here. It is a part of sql_request and sql_response as I remember. And it should be used in execute.c, not inside streams. You will see how much simpler the code will be.