From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [217.69.128.35]) (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 0F3DA469710 for ; Sat, 21 Nov 2020 18:17:10 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: Date: Sat, 21 Nov 2020 16:17:05 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 1/2] uuid: support comparison of uuid values in Lua List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: olegrok@tarantool.org, lvasiliev@tarantool.org Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the patch! See 3 comments below. On 18.11.2020 08:56, olegrok@tarantool.org wrote: > From: Oleg Babin > > Since Tarantool has uuid data type sometimes we want to compare > uuid vaues as it's possible for primitive types and decimals. This 1. vaues -> values. > patch exports function for uuid comparison and implements le and > lt metamethods for uuid type. > > Closes #5511 > > @TarantoolBot document > Title: uuid comparison is supported > > Currently comparison between uuid values is supported. > Example: > ```lua > u1 = uuid.fromstr('aaaaaaaa-aaaa-4000-b000-000000000001') > u2 = uuid.fromstr('bbbbbbbb-bbbb-4000-b000-000000000001') > > u1 > u2 -- false > u1 >= u2 -- false > u1 <= u2 -- true > u1 < u2 -- true > ``` > --- > Issue: https://github.com/tarantool/tarantool/issues/5511 > Branch: https://github.com/tarantool/tarantool/tree/olegrok/5511-uuid-cmp-v2 > > src/exports.h | 1 + > src/lua/uuid.lua | 25 +++++++++++++ > test/app/uuid.result | 79 ++++++++++++++++++++++++++++++++++++++++++ > test/app/uuid.test.lua | 29 ++++++++++++++++ > 4 files changed, 134 insertions(+) > > diff --git a/src/exports.h b/src/exports.h > index 867a027dc..ffbb84e3b 100644 > --- a/src/exports.h > +++ b/src/exports.h > @@ -508,6 +508,7 @@ EXPORT(tnt_iconv_close) > EXPORT(tnt_iconv_open) > EXPORT(tt_uuid_bswap) > EXPORT(tt_uuid_create) > +EXPORT(tt_uuid_compare) 2. Please, keep the lines sorted, as it is asked in the first lines of this file. 'r' > 'o', so tt_uuid_compare should be before tt_uuid_create. > EXPORT(tt_uuid_from_string) > EXPORT(tt_uuid_is_equal) > EXPORT(tt_uuid_is_nil) > diff --git a/src/lua/uuid.lua b/src/lua/uuid.lua > index 42016601d..08991cfeb 100644 > --- a/src/lua/uuid.lua > +++ b/src/lua/uuid.lua > @@ -118,9 +120,32 @@ local uuid_new_str = function() > return uuid_tostring(uuidbuf) > end > > +local check_uuid = function(value, index) > + if is_uuid(value) then > + return value > + end > + > + local err_fmt = 'incorrect value to compare with uuid as %d argument' > + error(err_fmt:format(index), 0) 3. Why did you use 0? You loose the error location then. I changed it to 4, and got more informative messages, showing the error location: [001] 'abc' <= u1 [001] --- [001] -- error: incorrect value to compare with uuid as 1 argument [001] +- error: '[string "return ''abc'' <= u1 "]:1: incorrect value to compare with uuid [001] + as 1 argument' I also noticed just now, that we already have uuid_eq, which looks different. I think you should unify the comparators, while you are here. Otherwise eq has its own 'check_uuid', and here you add a second one. It is inconsistent, even error messages look different. Also you anyway change it in the next commit, so it should not increase diff much.