From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 3 Apr 2019 18:40:03 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] Re: [PATCH v1 1/1] Implement mp_stack_top for mp_stack class Message-ID: <20190403154003.kdpt3vgbqtch3sjg@esperanza> References: <316cea905dca2ac8db8b1adb9d0040a9c338dc5c.1554219130.git.kshcherbatov@tarantool.org> <20190403121202.w66dbmhr7yrr5ysd@esperanza> <4377f916-eb85-44c4-2124-cb4b14d5a6d4@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4377f916-eb85-44c4-2124-cb4b14d5a6d4@tarantool.org> To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org List-ID: 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.