Re: [Tarantool-patches] [PATCH 9/9] util: introduce binary heap data structure

[Oleg Babin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for your fixes!

I found you've missed to add new file to "vshard/CMakeLists.txt" [1]


[1] https://github.com/tarantool/vshard/blob/master/vshard/CMakeLists.txt#L9

On 11/02/2021 01:36, Vladislav Shpilevoy wrote:
> Thanks for the review!
>
> On 10.02.2021 10:01, Oleg Babin wrote:
>> Thanks for your patch.
>>
>> Shouldn't it be added to storage "MODULE_INTERNALS" ?
> Hm. Not sure I understand. Did you mean 'vshard_modules' variable in
> storage/init.lua? Why? The heap is not used in storage/init.lua and
> won't be used there directly in future patches. The next patches
> will introduce new modules for storage/, which will use the heap,
> and will reload it.
>
> Also it does not have any global objects. So it does not
> need its own global M, if this is what you meant.

Yes, thanks for your answer. Got it.

>>> diff --git a/test/unit-tap/heap.test.lua b/test/unit-tap/heap.test.lua
>>> new file mode 100755
>>> index 0000000..8c3819f
>>> --- /dev/null
>>> +++ b/test/unit-tap/heap.test.lua
>>> @@ -0,0 +1,310 @@
>>> +#!/usr/bin/env tarantool
>>> +
>>> +local tap = require('tap')
>>> +local test = tap.test("cfg")
>>> +local heap = require('vshard.heap')
>>> +
>> Maybe it's better to use single brackets everywhere: test("cfg") -> test('cfg'). Or does such difference have some sense?
> Yeah, didn't notice it. Here is the diff:
>
> ====================
> diff --git a/test/unit-tap/heap.test.lua b/test/unit-tap/heap.test.lua
> index 8c3819f..9202f62 100755
> --- a/test/unit-tap/heap.test.lua
> +++ b/test/unit-tap/heap.test.lua
> @@ -1,7 +1,7 @@
>   #!/usr/bin/env tarantool
>   
>   local tap = require('tap')
> -local test = tap.test("cfg")
> +local test = tap.test('cfg')
>   local heap = require('vshard.heap')
>   
>   --
> @@ -109,7 +109,7 @@ local function test_min_heap_basic(test)
>           until not next_permutation(indexes)
>       end
>   
> -    test:ok(true, "no asserts")
> +    test:ok(true, 'no asserts')
>   end
>   
>   --
> @@ -143,7 +143,7 @@ local function test_max_heap_basic(test)
>           until not next_permutation(indexes)
>       end
>   
> -    test:ok(true, "no asserts")
> +    test:ok(true, 'no asserts')
>   end
>   
>   --
> @@ -178,7 +178,7 @@ local function test_min_heap_update_top(test)
>           until not next_permutation(indexes)
>       end
>   
> -    test:ok(true, "no asserts")
> +    test:ok(true, 'no asserts')
>   end
>   
>   --
> @@ -219,7 +219,7 @@ local function test_min_heap_update(test)
>           end
>       end
>   
> -    test:ok(true, "no asserts")
> +    test:ok(true, 'no asserts')
>   end
>   
>   --
> @@ -257,7 +257,7 @@ local function test_max_heap_delete(test)
>           end
>       end
>   
> -    test:ok(true, "no asserts")
> +    test:ok(true, 'no asserts')
>   end
>   
>   local function test_min_heap_remove_top(test)
> @@ -273,7 +273,7 @@ local function test_min_heap_remove_top(test)
>       end
>       assert(h:count() == 0)
>   
> -    test:ok(true, "no asserts")
> +    test:ok(true, 'no asserts')
>   end
>   
>   local function test_max_heap_remove_try(test)
> @@ -294,7 +294,7 @@ local function test_max_heap_remove_try(test)
>       assert(obj.index == -1)
>       assert(h:count() == 1)
>   
> -    test:ok(true, "no asserts")
> +    test:ok(true, 'no asserts')
>   end
>   
>   test:plan(7)