[PATCH v1 1/1] Implement mp_stack_top for mp_stack class

[PATCH v1 1/1] Implement mp_stack_top for mp_stack class

[Kirill Shcherbatov]

Introduced a new mp_stack_top method for mp_stack class to
return the pointer to a top frame of the stack.

This is required in scope of multikey indexes to keep a pointer
to a multikey frame and extract currently processed item index
of this frame later.

Needed for https://github.com/tarantool/tarantool/issues/1257
---
https://github.com/tarantool/msgpuck/tree/kshch/gh-1257-new-mpstacktop-method
https://github.com/tarantool/tarantool/issues/1257

 msgpuck.h | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/msgpuck.h b/msgpuck.h
index eab1339..c91d5d1 100644
--- a/msgpuck.h
+++ b/msgpuck.h
@@ -1278,6 +1278,14 @@ mp_stack_is_empty(struct mp_stack *stack);
 MP_PROTO bool
 mp_stack_is_full(struct mp_stack *stack);
 
+/**
+ * \brief Return the top mp_stack \a stack frame.
+ * \param stack - the pointer to a mp_stack to operate with.
+ * \pre mp_stack_is_empty(stack) == false
+ */
+MP_PROTO struct mp_frame *
+mp_stack_top(struct mp_stack *stack);
+
 /**
  * \brief Pop the top mp_stack \a stack frame.
  * \param stack - the pointer to a mp_stack to operate with.
@@ -2349,6 +2357,12 @@ mp_stack_is_full(struct mp_stack *stack)
 	return stack->used >= stack->size;
 }
 
+MP_IMPL struct mp_frame *
+mp_stack_top(struct mp_stack *stack)
+{
+	return &stack->frames[stack->used - 1];
+}
+
 MP_IMPL void
 mp_stack_pop(struct mp_stack *stack)
 {
@@ -2370,20 +2384,20 @@ MP_IMPL enum mp_type
 mp_stack_type(struct mp_stack *stack)
 {
 	assert(!mp_stack_is_empty(stack));
-	return stack->frames[stack->used - 1].type;
+	return mp_stack_top(stack)->type;
 }
 
 MP_IMPL int
 mp_stack_count(struct mp_stack *stack)
 {
-	return stack->frames[stack->used - 1].count;
+	return mp_stack_top(stack)->count;
 }
 
 MP_IMPL int
 mp_stack_advance(struct mp_stack *stack)
 {
 	assert(!mp_stack_is_empty(stack));
-	struct mp_frame *frame = &stack->frames[stack->used - 1];
+	struct mp_frame *frame = mp_stack_top(stack);
 	if (frame->curr < frame->count - 1)
 		return ++frame->curr;
 	return -1;
-- 
2.21.0

Re: [PATCH v1 1/1] Implement mp_stack_top for mp_stack class

[Vladimir Davydov]

On Tue, Apr 02, 2019 at 06:49:57PM +0300, Kirill Shcherbatov wrote:
> Introduced a new mp_stack_top method for mp_stack class to
> return the pointer to a top frame of the stack.
> 
> This is required in scope of multikey indexes to keep a pointer
> to a multikey frame and extract currently processed item index
> of this frame later.
> 
> Needed for https://github.com/tarantool/tarantool/issues/1257
> ---
> https://github.com/tarantool/msgpuck/tree/kshch/gh-1257-new-mpstacktop-method
> https://github.com/tarantool/tarantool/issues/1257
> 
>  msgpuck.h | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/msgpuck.h b/msgpuck.h
> index eab1339..c91d5d1 100644
> --- a/msgpuck.h
> +++ b/msgpuck.h
> @@ -1278,6 +1278,14 @@ mp_stack_is_empty(struct mp_stack *stack);
>  MP_PROTO bool
>  mp_stack_is_full(struct mp_stack *stack);
>  
> +/**
> + * \brief Return the top mp_stack \a stack frame.
> + * \param stack - the pointer to a mp_stack to operate with.
> + * \pre mp_stack_is_empty(stack) == false
> + */
> +MP_PROTO struct mp_frame *
> +mp_stack_top(struct mp_stack *stack);

Since we now have mp_stack_top, I think we should drop mp_stack_count
and mp_stack_type, otherwise there are two equally correct ways to do
the same thing, which is confusing:

	frame = mp_stack_top(stack);
	return frame->type

or

	return mp_stack_type(stack);

Similarly, I think we should rework mp_stack_advance so that it works
with mp_frame rather than stack and returns true/false rather than the
frame index. In other words, change

	int idx = mp_stack_advance(stack);
	if (idx < 0) {
		mp_stack_pop(stack);
		continue;
	}

to

	struct mp_frame *frame = mp_stack_frame(stack);
	if (!mp_frame_advance(frame)) {
		mp_stack_pop(stack);
		continue;
	}
	int idx = frame->curr;

or something like that. BTW, if you agree, let's also rename 'curr' to
'idx' or 'index' - 'curr' looks kinda ugly and since now we are going to
access it directly, we'd better rename it IMO.

What do you think?

Re: [tarantool-patches] Re: [PATCH v1 1/1] Implement mp_stack_top for mp_stack class

[Kirill Shcherbatov]

> or something like that. BTW, if you agree, let's also rename 'curr' to
> 'idx' or 'index' - 'curr' looks kinda ugly and since now we are going to
> access it directly, we'd better rename it IMO.
> 
> What do you think?

I am not shure that it is good concept. Consider a part of MP_PRINT macro

	while (!mp_stack_is_empty(&stack)) {
		struct mp_frame *frame = mp_stack_top(&stack);
		enum mp_type type = frame->type;
		bool stop = !mp_frame_advance(frame);
		if (frame->idx == 0 || frame->count == 0)
			PRINTF(type == MP_ARRAY ? "[" : "{");
		if (stop) {
			PRINTF(type == MP_ARRAY ? "]" : "}");
			mp_stack_pop(&stack);
			continue;
		} else if (frame->idx != 0) {
			PRINTF(type == MP_MAP && frame->idx % 2 == 1 ? ": " : ", ");
		}
		goto next;
	}

To my opinion, the code got worse.

Re: [tarantool-patches] Re: [PATCH v1 1/1] Implement mp_stack_top for mp_stack class

[Vladimir Davydov]

On Wed, Apr 03, 2019 at 06:16:25PM +0300, Kirill Shcherbatov wrote:
> > or something like that. BTW, if you agree, let's also rename 'curr' to
> > 'idx' or 'index' - 'curr' looks kinda ugly and since now we are going to
> > access it directly, we'd better rename it IMO.
> > 
> > What do you think?
> 
> I am not shure that it is good concept. Consider a part of MP_PRINT macro
> 
> 	while (!mp_stack_is_empty(&stack)) {
> 		struct mp_frame *frame = mp_stack_top(&stack);
> 		enum mp_type type = frame->type;
> 		bool stop = !mp_frame_advance(frame);
> 		if (frame->idx == 0 || frame->count == 0)
> 			PRINTF(type == MP_ARRAY ? "[" : "{");
> 		if (stop) {
> 			PRINTF(type == MP_ARRAY ? "]" : "}");
> 			mp_stack_pop(&stack);
> 			continue;
> 		} else if (frame->idx != 0) {
> 			PRINTF(type == MP_MAP && frame->idx % 2 == 1 ? ": " : ", ");
> 		}
> 		goto next;
> 	}
> 
> To my opinion, the code got worse.

Yeah, you're right, with the 'stop' flag it doesn't look good.
However, we could rewrite it in a slightly different fashion.
So here's what we have now for comparison:

	while (!mp_stack_is_empty(&stack)) {
		enum mp_type type = mp_stack_type(&stack);
		int curr = mp_stack_advance(&stack);
		if (curr == 0 || mp_stack_count(&stack) == 0)
			PRINTF(type == MP_ARRAY ? "[" : "{");
		if (curr == -1) {
			PRINTF(type == MP_ARRAY ? "]" : "}");
			mp_stack_pop(&stack);
			continue;
		} else if (curr != 0) {
			PRINTF(type == MP_MAP && curr % 2 == 1 ? ": " : ", ");
		}
		goto next;
	}

Here's what we could turn this into using the new API:

	while (!mp_stack_is_empty(&stack)) {
		enum mp_frame *frame = mp_stack_top(&stack);
		if (frame->idx < 0)
			PRINTF(type == MP_ARRAY ? "[" : "{");
		if (!mp_frame_advance(frame)) {
			PRINTF(type == MP_ARRAY ? "]" : "}");
			mp_stack_pop(&stack);
			continue;
		} else if (frame->idx != 0) {
			PRINTF(type == MP_MAP && frame->idx % 2 == 1 ? ": " : ", ");
		}
		goto next;
	}

Now it's not that bad, don't you agree? We even managed to get rid of
the empty array/map check.

Re: [tarantool-patches] Re: [PATCH v1 1/1] Implement mp_stack_top for mp_stack class

[Kirill Shcherbatov]

> Now it's not that bad, don't you agree? We even managed to get rid of
> the empty array/map check.

Yep. Looks good. On the branch.

====================================================

Introduced a new mp_stack_top method for mp_stack class to
return the pointer to a top frame of the stack.

Class methods have been redesigned so that this new mp_stack_top
helper fits into the API naturally.

This is required in scope of multikey indexes to keep a pointer
to a multikey frame and extract currently processed item index
of this frame later.

Needed for https://github.com/tarantool/tarantool/issues/1257
---
 msgpuck.c | 16 ++++++------
 msgpuck.h | 76 ++++++++++++++++++++-----------------------------------
 2 files changed, 36 insertions(+), 56 deletions(-)

diff --git a/msgpuck.c b/msgpuck.c
index 05e472e..d0ffb83 100644
--- a/msgpuck.c
+++ b/msgpuck.c
@@ -306,16 +306,16 @@ next:										\
 		return -1;							\
 	}									\
 	while (!mp_stack_is_empty(&stack)) {					\
-		enum mp_type type = mp_stack_type(&stack);			\
-		int curr = mp_stack_advance(&stack);				\
-		if (curr == 0 || mp_stack_count(&stack) == 0)			\
-			PRINTF(type == MP_ARRAY ? "[" : "{");			\
-		if (curr == -1) {						\
-			PRINTF(type == MP_ARRAY ? "]" : "}");			\
+		struct mp_frame *frame = mp_stack_top(&stack);			\
+		if (frame->idx < 0)						\
+			PRINTF(frame->type == MP_ARRAY ? "[" : "{");		\
+		if (!mp_frame_advance(frame)) {					\
+			PRINTF(frame->type == MP_ARRAY ? "]" : "}");		\
 			mp_stack_pop(&stack);					\
 			continue;						\
-		} else if (curr != 0) {						\
-			PRINTF(type == MP_MAP && curr % 2 == 1 ? ": " : ", ");	\
+		} else if (frame->idx != 0) {					\
+			PRINTF(frame->type == MP_MAP && frame->idx % 2 == 1 ?	\
+			       ": " : ", ");					\
 		}								\
 		goto next;							\
 	}									\
diff --git a/msgpuck.h b/msgpuck.h
index eab1339..64b10e6 100644
--- a/msgpuck.h
+++ b/msgpuck.h
@@ -1222,7 +1222,7 @@ struct mp_frame {
 	 * Index of currently processing item. Must be less than
 	 * mp_frame::count member.
 	 */
-	int curr;
+	int idx;
 };
 
 /**
@@ -1278,6 +1278,14 @@ mp_stack_is_empty(struct mp_stack *stack);
 MP_PROTO bool
 mp_stack_is_full(struct mp_stack *stack);
 
+/**
+ * \brief Return the top mp_stack \a stack frame.
+ * \param stack - the pointer to a mp_stack to operate with.
+ * \pre mp_stack_is_empty(stack) == false
+ */
+MP_PROTO struct mp_frame *
+mp_stack_top(struct mp_stack *stack);
+
 /**
  * \brief Pop the top mp_stack \a stack frame.
  * \param stack - the pointer to a mp_stack to operate with.
@@ -1298,33 +1306,13 @@ MP_PROTO void
 mp_stack_push(struct mp_stack *stack, enum mp_type type, int count);
 
 /**
- * \brief Get type attribute of the \a stack top frame.
- * \param stack - the pointer to a stack to operate with.
- * \retval enum mp_type value - the top stack frame type.
- * \pre mp_stack_is_empty(stack) == false
- */
-MP_PROTO enum mp_type
-mp_stack_type(struct mp_stack *stack);
-
-/**
- * \brief Get count attribute of the \a stack top frame.
- * \param stack - the pointer to a stack to operate with.
- * \retval count - the top stack frame items count.
- * \pre mp_stack_is_empty(stack) == false
- */
-MP_PROTO int
-mp_stack_count(struct mp_stack *stack);
-
-/**
- * \brief Advance curr attribute of the \a stack top frame.
- * \param stack - the pointer to a stack to operate with.
- * \retval index of the element to process if the top
- *         mp_frame::curr is less than top mp_frame::count field.
- *         -1 otherwise.
- * \pre mp_stack_is_empty(stack) == false
+ * \brief Advance idx attribute of the \a frame.
+ * \param frame - the frame pointer to operate with.
+ * \retval true when mp_frame::idx is less than  mp_frame::count.
+ *         false otherwise.
  */
-MP_PROTO int
-mp_stack_advance(struct mp_stack *stack);
+MP_PROTO bool
+mp_frame_advance(struct mp_frame *frame);
 
 /*
  * }}}
@@ -2349,6 +2337,12 @@ mp_stack_is_full(struct mp_stack *stack)
 	return stack->used >= stack->size;
 }
 
+MP_IMPL struct mp_frame *
+mp_stack_top(struct mp_stack *stack)
+{
+	return &stack->frames[stack->used - 1];
+}
+
 MP_IMPL void
 mp_stack_pop(struct mp_stack *stack)
 {
@@ -2363,30 +2357,16 @@ mp_stack_push(struct mp_stack *stack, enum mp_type type, int count)
 	int idx = stack->used++;
 	stack->frames[idx].type = type;
 	stack->frames[idx].count = count;
-	stack->frames[idx].curr = -1;
-}
-
-MP_IMPL enum mp_type
-mp_stack_type(struct mp_stack *stack)
-{
-	assert(!mp_stack_is_empty(stack));
-	return stack->frames[stack->used - 1].type;
+	stack->frames[idx].idx = -1;
 }
 
-MP_IMPL int
-mp_stack_count(struct mp_stack *stack)
-{
-	return stack->frames[stack->used - 1].count;
-}
-
-MP_IMPL int
-mp_stack_advance(struct mp_stack *stack)
+MP_IMPL bool
+mp_frame_advance(struct mp_frame *frame)
 {
-	assert(!mp_stack_is_empty(stack));
-	struct mp_frame *frame = &stack->frames[stack->used - 1];
-	if (frame->curr < frame->count - 1)
-		return ++frame->curr;
-	return -1;
+	if (frame->idx >= frame->count - 1)
+		return false;
+	++frame->idx;
+	return true;
 }
 
 /** \endcond */
-- 
2.21.0

Re: [tarantool-patches] Re: [PATCH v1 1/1] Implement mp_stack_top for mp_stack class

[Vladimir Davydov]

On Wed, Apr 03, 2019 at 08:49:23PM +0300, Kirill Shcherbatov wrote:
> @@ -2349,6 +2337,12 @@ mp_stack_is_full(struct mp_stack *stack)
>  	return stack->used >= stack->size;
>  }
>  
> +MP_IMPL struct mp_frame *
> +mp_stack_top(struct mp_stack *stack)
> +{

Nit:
	assert(!mp_stack_is_empty(stack));

> +	return &stack->frames[stack->used - 1];
> +}

Other than that, the patch looks good, BUT for some reason it doesn't
compile on CentOS 6 on Travis, see

  https://travis-ci.org/tarantool/msgpuck/jobs/515314874

It fails with

  BFD: /build/BUILDROOT/msgpuck-2.0.23-1.el6.x86_64/usr/lib64/libmsgpuck.a(msgpuck.c.o): invalid relocation type 42

Please figure out why is that and fix if possible.

Re: [tarantool-patches] Re: [PATCH v1 1/1] Implement mp_stack_top for mp_stack class

[Kirill Shcherbatov]

> Nit:
> 	assert(!mp_stack_is_empty(stack));
Tnx.

> Other than that, the patch looks good, BUT for some reason it doesn't
> compile on CentOS 6 on Travis, see
> 
>   https://travis-ci.org/tarantool/msgpuck/jobs/515314874
> 
> It fails with
> 
>   BFD: /build/BUILDROOT/msgpuck-2.0.23-1.el6.x86_64/usr/lib64/libmsgpuck.a(msgpuck.c.o): invalid relocation type 42
> 
> Please figure out why is that and fix if possible.
Answered on other branch.

============================================================

Introduced a new mp_stack_top method for mp_stack class to
return the pointer to a top frame of the stack.

Class methods have been redesigned so that this new mp_stack_top
helper fits into the API naturally.

This is required in scope of multikey indexes to keep a pointer
to a multikey frame and extract currently processed item index
of this frame later.

Needed for https://github.com/tarantool/tarantool/issues/1257
---
 msgpuck.c | 16 ++++++------
 msgpuck.h | 77 +++++++++++++++++++++----------------------------------
 2 files changed, 37 insertions(+), 56 deletions(-)

diff --git a/msgpuck.c b/msgpuck.c
index 05e472e..d0ffb83 100644
--- a/msgpuck.c
+++ b/msgpuck.c
@@ -306,16 +306,16 @@ next:										\
 		return -1;							\
 	}									\
 	while (!mp_stack_is_empty(&stack)) {					\
-		enum mp_type type = mp_stack_type(&stack);			\
-		int curr = mp_stack_advance(&stack);				\
-		if (curr == 0 || mp_stack_count(&stack) == 0)			\
-			PRINTF(type == MP_ARRAY ? "[" : "{");			\
-		if (curr == -1) {						\
-			PRINTF(type == MP_ARRAY ? "]" : "}");			\
+		struct mp_frame *frame = mp_stack_top(&stack);			\
+		if (frame->idx < 0)						\
+			PRINTF(frame->type == MP_ARRAY ? "[" : "{");		\
+		if (!mp_frame_advance(frame)) {					\
+			PRINTF(frame->type == MP_ARRAY ? "]" : "}");		\
 			mp_stack_pop(&stack);					\
 			continue;						\
-		} else if (curr != 0) {						\
-			PRINTF(type == MP_MAP && curr % 2 == 1 ? ": " : ", ");	\
+		} else if (frame->idx != 0) {					\
+			PRINTF(frame->type == MP_MAP && frame->idx % 2 == 1 ?	\
+			       ": " : ", ");					\
 		}								\
 		goto next;							\
 	}									\
diff --git a/msgpuck.h b/msgpuck.h
index eab1339..2d39b40 100644
--- a/msgpuck.h
+++ b/msgpuck.h
@@ -1222,7 +1222,7 @@ struct mp_frame {
 	 * Index of currently processing item. Must be less than
 	 * mp_frame::count member.
 	 */
-	int curr;
+	int idx;
 };
 
 /**
@@ -1278,6 +1278,14 @@ mp_stack_is_empty(struct mp_stack *stack);
 MP_PROTO bool
 mp_stack_is_full(struct mp_stack *stack);
 
+/**
+ * \brief Return the top mp_stack \a stack frame.
+ * \param stack - the pointer to a mp_stack to operate with.
+ * \pre mp_stack_is_empty(stack) == false
+ */
+MP_PROTO struct mp_frame *
+mp_stack_top(struct mp_stack *stack);
+
 /**
  * \brief Pop the top mp_stack \a stack frame.
  * \param stack - the pointer to a mp_stack to operate with.
@@ -1298,33 +1306,13 @@ MP_PROTO void
 mp_stack_push(struct mp_stack *stack, enum mp_type type, int count);
 
 /**
- * \brief Get type attribute of the \a stack top frame.
- * \param stack - the pointer to a stack to operate with.
- * \retval enum mp_type value - the top stack frame type.
- * \pre mp_stack_is_empty(stack) == false
- */
-MP_PROTO enum mp_type
-mp_stack_type(struct mp_stack *stack);
-
-/**
- * \brief Get count attribute of the \a stack top frame.
- * \param stack - the pointer to a stack to operate with.
- * \retval count - the top stack frame items count.
- * \pre mp_stack_is_empty(stack) == false
- */
-MP_PROTO int
-mp_stack_count(struct mp_stack *stack);
-
-/**
- * \brief Advance curr attribute of the \a stack top frame.
- * \param stack - the pointer to a stack to operate with.
- * \retval index of the element to process if the top
- *         mp_frame::curr is less than top mp_frame::count field.
- *         -1 otherwise.
- * \pre mp_stack_is_empty(stack) == false
+ * \brief Advance idx attribute of the \a frame.
+ * \param frame - the frame pointer to operate with.
+ * \retval true when mp_frame::idx is less than  mp_frame::count.
+ *         false otherwise.
  */
-MP_PROTO int
-mp_stack_advance(struct mp_stack *stack);
+MP_PROTO bool
+mp_frame_advance(struct mp_frame *frame);
 
 /*
  * }}}
@@ -2349,6 +2337,13 @@ mp_stack_is_full(struct mp_stack *stack)
 	return stack->used >= stack->size;
 }
 
+MP_IMPL struct mp_frame *
+mp_stack_top(struct mp_stack *stack)
+{
+	assert(!mp_stack_is_empty(stack));
+	return &stack->frames[stack->used - 1];
+}
+
 MP_IMPL void
 mp_stack_pop(struct mp_stack *stack)
 {
@@ -2363,30 +2358,16 @@ mp_stack_push(struct mp_stack *stack, enum mp_type type, int count)
 	int idx = stack->used++;
 	stack->frames[idx].type = type;
 	stack->frames[idx].count = count;
-	stack->frames[idx].curr = -1;
-}
-
-MP_IMPL enum mp_type
-mp_stack_type(struct mp_stack *stack)
-{
-	assert(!mp_stack_is_empty(stack));
-	return stack->frames[stack->used - 1].type;
-}
-
-MP_IMPL int
-mp_stack_count(struct mp_stack *stack)
-{
-	return stack->frames[stack->used - 1].count;
+	stack->frames[idx].idx = -1;
 }
 
-MP_IMPL int
-mp_stack_advance(struct mp_stack *stack)
+MP_IMPL bool
+mp_frame_advance(struct mp_frame *frame)
 {
-	assert(!mp_stack_is_empty(stack));
-	struct mp_frame *frame = &stack->frames[stack->used - 1];
-	if (frame->curr < frame->count - 1)
-		return ++frame->curr;
-	return -1;
+	if (frame->idx >= frame->count - 1)
+		return false;
+	++frame->idx;
+	return true;
 }
 
 /** \endcond */
-- 
2.21.0

Re: [tarantool-patches] Re: [PATCH v1 1/1] Implement mp_stack_top for mp_stack class

[Vladimir Davydov]

On Fri, Apr 05, 2019 at 08:17:23PM +0300, Kirill Shcherbatov wrote:
> Introduced a new mp_stack_top method for mp_stack class to
> return the pointer to a top frame of the stack.
> 
> Class methods have been redesigned so that this new mp_stack_top
> helper fits into the API naturally.
> 
> This is required in scope of multikey indexes to keep a pointer
> to a multikey frame and extract currently processed item index
> of this frame later.
> 
> Needed for https://github.com/tarantool/tarantool/issues/1257
> ---
>  msgpuck.c | 16 ++++++------
>  msgpuck.h | 77 +++++++++++++++++++++----------------------------------
>  2 files changed, 37 insertions(+), 56 deletions(-)

Pushed to tarantool-msgpuck:master.