From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: Re: [PATCH v1 1/1] Implement mp_stack_top for mp_stack class
Date: Wed, 3 Apr 2019 15:12:02 +0300 [thread overview]
Message-ID: <20190403121202.w66dbmhr7yrr5ysd@esperanza> (raw)
In-Reply-To: <316cea905dca2ac8db8b1adb9d0040a9c338dc5c.1554219130.git.kshcherbatov@tarantool.org>
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?
next prev parent reply other threads:[~2019-04-03 12:12 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-02 15:49 Kirill Shcherbatov
2019-04-03 12:12 ` Vladimir Davydov [this message]
2019-04-03 15:16 ` [tarantool-patches] " Kirill Shcherbatov
2019-04-03 15:40 ` Vladimir Davydov
2019-04-03 17:49 ` Kirill Shcherbatov
2019-04-04 15:42 ` Vladimir Davydov
2019-04-05 17:17 ` Kirill Shcherbatov
2019-04-07 12:02 ` Vladimir Davydov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190403121202.w66dbmhr7yrr5ysd@esperanza \
--to=vdavydov.dev@gmail.com \
--cc=kshcherbatov@tarantool.org \
--cc=tarantool-patches@freelists.org \
--subject='Re: [PATCH v1 1/1] Implement mp_stack_top for mp_stack class' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox