From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 59A0C42F4AE for ; Fri, 19 Jun 2020 11:51:15 +0300 (MSK) Date: Fri, 19 Jun 2020 11:50:27 +0300 From: Alexander Turenko Message-ID: <20200619085027.kcghsk32q4gad5zq@tkn_work_nb> References: <811cb85f-f9e5-f353-68bb-010081d85eb2@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <811cb85f-f9e5-f353-68bb-010081d85eb2@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 1/3] merger: fix NULL dereference when called via iproto List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org > > static int > > -luaL_merge_source_buffer_fetch(struct merge_source_buffer *source) > > +luaL_merge_source_buffer_fetch_impl(struct lua_State *L, > > + struct merge_source_buffer *source) > > I see my comment to the former first commit was missed somewhy, so I say > it here: Sorry, I misread it a bit and understood as part of illustration that there are still no consistency even after dropping luaL prefix. > > - If a function takes lua_State, it is not mean it should get > luaL_ prefix. Since the first commit was dropped, this comment > we can't fix. > > - If a function is a method of struct, struct pointer goes *always* > first, like 'this' argument in C++. Present of lua_State does > not mean it should be first. This can and should be fixed, I think. > > This function in the first place is a method of merge_source_buffer. > So it should take this argument first. luaL_merge_source_buffer_fetch_impl() > currently is the only method of merge_source_buffer, which takes > merge_source_buffer not in the first argument. It means, I don't see > how the consistency is improved here. The same for > luaL_merge_source_table_fetch, luaL_merge_source_table_next_impl, > luaL_merge_source_tuple_fetch, luaL_merge_source_tuple_next_impl. Moved to the end of an argument list. Removed the following paragraph from the commit message: | Changed order of luaL_merge_source_tuple_fetch() arguments to unify it | with other functions (, ).