[Tarantool-patches] [PATCH] rtree: add comments on ignored rtree_search() return value

[Tarantool-patches] [PATCH] rtree: add comments on ignored rtree_search() return value

[Ilya Kosarev]

rtree_search() has return value and it is ignored in some cases.
Although it is totally fine it seems to be reasonable to comment those
cases as far as such usage might be questionable.

Closes #2052
---
Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-2052-rtree_search()-result-ignored
Issue: https://github.com/tarantool/tarantool/issues/2052

 src/box/memtx_rtree.c       | 7 +++++++
 test/unit/rtree_iterator.cc | 7 +++++++
 2 files changed, 14 insertions(+)

diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c
index 612fcb2a9f..6b8b0a2e17 100644
--- a/src/box/memtx_rtree.c
+++ b/src/box/memtx_rtree.c
@@ -322,6 +322,13 @@ memtx_rtree_index_create_iterator(struct index *base,  enum iterator_type type,
 	it->base.next = index_rtree_iterator_next;
 	it->base.free = index_rtree_iterator_free;
 	rtree_iterator_init(&it->impl);
+	/*
+	 * We don't care if rtree_search() does or does not find anything
+	 * as far as we are fine anyway: iterator is initialized correctly
+	 * and the case where nothing is found and rtree_iterator_next()
+	 * returns NULL should be processed in the correct way on the
+	 * caller's side.
+	 */
 	rtree_search(&index->tree, &rect, op, &it->impl);
 	return (struct iterator *)it;
 }
diff --git a/test/unit/rtree_iterator.cc b/test/unit/rtree_iterator.cc
index b3c8695e9d..641b659177 100644
--- a/test/unit/rtree_iterator.cc
+++ b/test/unit/rtree_iterator.cc
@@ -274,6 +274,13 @@ iterator_invalidate_check()
 			rtree_insert(&tree, &rect, record_t(i+1));
 		}
 		rtree_set2d(&rect, 0, 0, test_size, test_size);
+		/*
+		 * We don't care if rtree_search() does or does not find
+		 * anything as far as we are fine anyway: iterator is
+		 * initialized correctly and the case where nothing is found
+		 * and rtree_iterator_next() returns NULL will be processed
+		 * correctly as fail case in check #19.
+		 */
 		rtree_search(&tree, &rect, SOP_BELONGS, &iterators[0]);
 		if (!rtree_iterator_next(&iterators[0])) {
 			fail("Integrity check failed (19)", "false");
-- 
2.17.1

Re: [Tarantool-patches] [PATCH] rtree: add comments on ignored rtree_search() return value

[Aleksandr Lyapunov]

Hi, thanks for the patch, LGTM

On 01.09.2020 19:27, Ilya Kosarev wrote:
> rtree_search() has return value and it is ignored in some cases.
> Although it is totally fine it seems to be reasonable to comment those
> cases as far as such usage might be questionable.
>
> Closes #2052
> ---
> Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-2052-rtree_search()-result-ignored
> Issue: https://github.com/tarantool/tarantool/issues/2052
>
>   src/box/memtx_rtree.c       | 7 +++++++
>   test/unit/rtree_iterator.cc | 7 +++++++
>   2 files changed, 14 insertions(+)
>
> diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c
> index 612fcb2a9f..6b8b0a2e17 100644
> --- a/src/box/memtx_rtree.c
> +++ b/src/box/memtx_rtree.c
> @@ -322,6 +322,13 @@ memtx_rtree_index_create_iterator(struct index *base,  enum iterator_type type,
>   	it->base.next = index_rtree_iterator_next;
>   	it->base.free = index_rtree_iterator_free;
>   	rtree_iterator_init(&it->impl);
> +	/*
> +	 * We don't care if rtree_search() does or does not find anything
> +	 * as far as we are fine anyway: iterator is initialized correctly
> +	 * and the case where nothing is found and rtree_iterator_next()
> +	 * returns NULL should be processed in the correct way on the
> +	 * caller's side.
> +	 */
>   	rtree_search(&index->tree, &rect, op, &it->impl);
>   	return (struct iterator *)it;
>   }
> diff --git a/test/unit/rtree_iterator.cc b/test/unit/rtree_iterator.cc
> index b3c8695e9d..641b659177 100644
> --- a/test/unit/rtree_iterator.cc
> +++ b/test/unit/rtree_iterator.cc
> @@ -274,6 +274,13 @@ iterator_invalidate_check()
>   			rtree_insert(&tree, &rect, record_t(i+1));
>   		}
>   		rtree_set2d(&rect, 0, 0, test_size, test_size);
> +		/*
> +		 * We don't care if rtree_search() does or does not find
> +		 * anything as far as we are fine anyway: iterator is
> +		 * initialized correctly and the case where nothing is found
> +		 * and rtree_iterator_next() returns NULL will be processed
> +		 * correctly as fail case in check #19.
> +		 */
>   		rtree_search(&tree, &rect, SOP_BELONGS, &iterators[0]);
>   		if (!rtree_iterator_next(&iterators[0])) {
>   			fail("Integrity check failed (19)", "false");

Re: [Tarantool-patches] [PATCH] rtree: add comments on ignored rtree_search() return value

[Kirill Yukhin]

Hello,

On 01 сен 19:27, Ilya Kosarev wrote:
> rtree_search() has return value and it is ignored in some cases.
> Although it is totally fine it seems to be reasonable to comment those
> cases as far as such usage might be questionable.
> 
> Closes #2052
> ---
> Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-2052-rtree_search()-result-ignored

In future, please avoid using of braces in branch names.

> Issue: https://github.com/tarantool/tarantool/issues/2052

LGTM.
I've checked your patch into 1.10, 2.4, 2.5 and master.

--
Regards, Kirill Yukhin