From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 806A06ECE3; Tue, 2 Nov 2021 14:35:30 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 806A06ECE3 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1635852930; bh=wP2UP1rDp0z5ga5ufdEWv2AMy1wB7oWJX91ykR2ZTqo=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=s72vGNM3uKGM7lLgWWXpTPnPLrLK+ZXWr/rjsUblgrpT4wBDb52NseAMWJd5RuO8d ldZ/8VpRSPveakcQ1Lrd8NC/w35mGPPv/2M0BrX+bDi8rkZjAXrovM0l61IaCtENOR JfKGv4E5AastAeR4b6HjJcoNBQM7oOpK3O4Ee32w= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (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 DE7C36ECE3 for ; Tue, 2 Nov 2021 14:35:28 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org DE7C36ECE3 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1mhs4e-0001Hm-5v; Tue, 02 Nov 2021 14:35:28 +0300 Date: Tue, 2 Nov 2021 14:35:27 +0300 To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Message-ID: <20211102113527.GA104458@tarantool.org> References: <7a6d7ca687a6e4d06b087af5f2e442042b38cf7b.1633713432.git.imeevma@gmail.com> <52e168e4-1559-fd6c-c5a6-d98e3c2d678a@tarantool.org> <20211025080212.GA36295@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD98893113B6235BE91E7EB397628F541CF2A4E523F4F395375182A05F5380850406902D3FEB2B91441BF71F927D12AEA47657CF008C6426C891969FEAC8E0FD428 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE73B44982FA5E78411EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006370BACBAB4C30C4AEB8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8669992F82EDEA4F3746DCB29C370E786117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BAA867293B0326636D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B62CFFCC7B69C47339089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A589A8B3C7FCAFEE3662CF1B7CA9C0938006EB724359C353F7D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA752DA3D96DA0CEF5C48E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D342B1F2AD168155B0653B72DE037F12315BE35B47F96D67F86EA7B740BBD79314A64BA3E4663D8E1911D7E09C32AA3244C9BA91DC1AC37B55111232D252350B5EE60759606DA2E136A729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojyQYxHpRvLO6gu7WLiKDYCA== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D380C22BE0638062D35D5017BFE775F1983D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v1 01/21] sql: refactor CHAR() function X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Mergen Imeev via Tarantool-patches Reply-To: Mergen Imeev Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi! Thank you for the review! My answer and diff below. Also, I fixed the argument types passed to the U8_* macros as they were throwing an error for some targents in CI. I believe this was due to the old ICU. On Sat, Oct 30, 2021 at 01:42:42AM +0200, Vladislav Shpilevoy wrote: > Hi! Thanks for the fixes! > > See 3 comments below. > > > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > > index afe34f7f0..dee28b852 100644 > > --- a/src/box/sql/func.c > > +++ b/src/box/sql/func.c > > @@ -717,6 +717,57 @@ func_substr_characters(struct sql_context *ctx, int argc, struct Mem *argv) > > ctx->is_aborted = true; > > } > > > > +/** > > + * Implementation of the CHAR() function. > > + * > > + * This function takes zero or more arguments, each of which is an integer. It > > + * constructs a string where each character of the string is the unicode > > + * character for the corresponding integer argument. > > + * > > + * If an argument is negative or greater than 0x10ffff, the symbol "�" is used. > > + * Symbol '\0' used instead of NULL argument. > > + */ > > +static void > > +func_char(struct sql_context *ctx, int argc, struct Mem *argv) > > +{ > > + if (argc == 0) > > + return mem_set_str_static(ctx->pOut, "", 0); > > + struct region *region = &fiber()->gc; > > + size_t svp = region_used(region); > > + UChar32 *buf = region_alloc(region, argc * sizeof(*buf)); > > 1. Would be better to use region_alloc_array(). Otherwise you > risk to get misaligned data. > Thanks, fixed. > > + if (buf == NULL) { > > + ctx->is_aborted = true; > > 2. Need to use diag_set() here. > Fixed. > > + return; > > + } > > + int len = 0; > > + for (int i = 0; i < argc; ++i) { > > + if (mem_is_null(&argv[i])) > > + buf[i] = 0; > > + else if (!mem_is_uint(&argv[i]) || argv[i].u.u > 0x10ffff) > > + buf[i] = 0xfffd; > > + else > > + buf[i] = argv[i].u.u; > > + len += U8_LENGTH(buf[i]); > > + } > > + > > + char *str = sqlDbMallocRawNN(sql_get(), len); > > + if (str == NULL) { > > + ctx->is_aborted = true; > > 3. region leaks here, doesn't it? True, fixed. Diff: diff --git a/src/box/sql/func.c b/src/box/sql/func.c index 71c46924e..0cd8f8f69 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -745,9 +745,11 @@ func_char(struct sql_context *ctx, int argc, struct Mem *argv) return mem_set_str_static(ctx->pOut, "", 0); struct region *region = &fiber()->gc; size_t svp = region_used(region); - UChar32 *buf = region_alloc(region, argc * sizeof(*buf)); + uint32_t size; + UChar32 *buf = region_alloc_array(region, typeof(*buf), argc, &size); if (buf == NULL) { ctx->is_aborted = true; + diag_set(OutOfMemory, size, "region_alloc_array", "buf"); return; } int len = 0; @@ -763,13 +765,14 @@ func_char(struct sql_context *ctx, int argc, struct Mem *argv) char *str = sqlDbMallocRawNN(sql_get(), len); if (str == NULL) { + region_truncate(region, svp); ctx->is_aborted = true; return; } int pos = 0; for (int i = 0; i < argc; ++i) { - bool is_error = false; - U8_APPEND(str, pos, len, buf[i], is_error); + UBool is_error = false; + U8_APPEND((uint8_t *)str, pos, len, buf[i], is_error); assert(!is_error); (void)is_error; }