From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp30.i.mail.ru (smtp30.i.mail.ru [94.100.177.90]) (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 2F413469710 for ; Tue, 24 Nov 2020 08:57:19 +0300 (MSK) References: <52439555-a382-189e-941a-22aa4b0e2683@tarantool.org> From: Oleg Babin Message-ID: <7e7582d6-6b55-4051-a640-880008e839d3@tarantool.org> Date: Tue, 24 Nov 2020 08:57:17 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB 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: Vladislav Shpilevoy , Igor Munkin Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for comments! On 24/11/2020 00:58, Vladislav Shpilevoy wrote: > Hi! Thanks for the fixes! > >> diff --git a/src/lua/uuid.lua b/src/lua/uuid.lua >> index 08991cfeb..275f4e5c5 100644 >> --- a/src/lua/uuid.lua >> +++ b/src/lua/uuid.lua >> @@ -93,13 +93,20 @@ local uuid_isnil = function(uu) >>      return builtin.tt_uuid_is_nil(uu) >>  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), 4) >> +end >> + >>  local uuid_eq = function(lhs, rhs) >>      if not is_uuid(rhs) then >>          return false >>      end >> -    if not is_uuid(lhs) then >> -        return error('Usage: uuid == var') >> -    end >> +    lhs = check_uuid(lhs, 1) > Check_uuid() here uses incorrect error index. check_uuid() > raises error(..., 4). So check_uuid() itself is level 1. > uuid_eq() is level 2. And the calling code is level 3. You > will raise one level beyond the caller code. > > For uuid_lt and uuid_le it works, because check_uuid() is > level 1, uuid_cmp() is level 2, uuid_lt/le() is level 3, and > the calling code is level 4. Good catch! Thanks. Fixed in the following way: diff --git a/src/lua/uuid.lua b/src/lua/uuid.lua index 275f4e5c5..1171dfcc4 100644 --- a/src/lua/uuid.lua +++ b/src/lua/uuid.lua @@ -93,20 +93,20 @@ local uuid_isnil = function(uu)      return builtin.tt_uuid_is_nil(uu)  end -local check_uuid = function(value, index) +local check_uuid = function(value, index, err_lvl)      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), 4) +    error(err_fmt:format(index), err_lvl)  end  local uuid_eq = function(lhs, rhs)      if not is_uuid(rhs) then          return false      end -    lhs = check_uuid(lhs, 1) +    lhs = check_uuid(lhs, 1, 3)      return builtin.tt_uuid_is_equal(lhs, rhs)  end @@ -128,8 +128,8 @@ local uuid_new_str = function()  end  local uuid_cmp = function(lhs, rhs) -    lhs = check_uuid(lhs, 1) -    rhs = check_uuid(rhs, 2) +    lhs = check_uuid(lhs, 1, 4) +    rhs = check_uuid(rhs, 2, 4)      return builtin.tt_uuid_compare(lhs, rhs)  end  local uuid_lt = function(lhs, rhs) > But I can't give you a test, since I have no idea how to provide > lhs with non-uuid type. Igor said we can give metatable of uuid to > some non-uuid value, but I failed to assign uuid metatable to > anything else. It's possible but I don't think it's really needed. I put small snippet below: uuid = require('uuid') eq = debug.getmetatable(uuid()).__eq eq('', uuid()) -- error: incorrect value to compare with uuid as 1 argument proxy = newproxy() debug.setmetatable(proxy, {__eq = eq}) proxy == uuid() -- error: '[string "return proxy == uuid()"]:1: incorrect value to compare with uuid as 1 argument' I can add it to our test suite but it looks really strange > After you fix this (but I don't insist on adding a test, because > I don't know how), I suggest you to get a second review from Igor. Ok. I add Igor to CC. Igor, could you provide the second review?