Re: [PATCH RFC 01/24] ref-filter: get rid of goto

From: Оля Тележная <olyatelezhnaya@gmail.com>
Date: Jan 29, 2018, 07:13 UTC
Message-ID: <CAL21BmkOGE6UZQJTFQjq3qnmywwBJQOUXE7jSwP1_Vn6PZ8ezA@mail.gmail.com>
In-Reply-To: <xmqqfu6s74pf.fsf@gitster.mtv.corp.google.com>

2018-01-26 23:19 GMT+03:00 Junio C Hamano <gitster@pobox.com>:

Show 12 quoted lines

> Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
>
>> Get rid of goto command in ref-filter for better readability.
>>
>> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored by: Jeff King <peff@peff.net>
>> ---
>
> How was this patch "mentored by" these two folks?  Have they already
> reviewed and gave you OK, or are you asking them to also help reviewing
> with this message?  Mostly just being curious.

Christian and Jeff help me when I have different sort of difficulties. Not sure that they were helping me with that commit separately. Both of them reviewed my code and said that it's ready for a final review (actually, Christian said, but it's usual situation when I ask for help/review and one of them helps me. The other one could add something, but, as I understand, if he totally agree, he will keep silence, and I find that behavior logical). Do I need to delete these lines from some of commits where I do not remember help from them?

Show 18 quoted lines

>
> It is not convincning that this splitting the last part of a single
> function into a separate helper function that is called from only
> one place improves readability.  If better readability is the
> purpose, I would even say
>
>          for (i = 0; i < used_atom_cnt; i++) {
>                 if (...)
> -                       goto need_obj;
> +                       break;
>         }
> -       return;
> +       if (used_atom_cnt <= i)
>                 return;
>
> -need_obj:
>
> would make the result easier to follow with a much less impact.

It's hard for me to read the code with goto, and as I know, it's not only my problem, it's usual situation to try to get rid of gotos. I always need to re-check whether we use that piece of code somewhere else or not, and how we do that. I also think that it's good that most of variables in the beginning of the function populate_value go to new function.

Show 5 quoted lines

>
> If we later in the series will use this new helper function from
> other places, it certainly makes sense to create a new helper like
> this patch does, but then "get rid of goto for readability" is not
> the justification for such a change.

We don't use that new function anywhere else further. So, I can delete this commit or I can change commit message (if so, please give me some ideas what I need to mention there).

Show 16 quoted lines

>
>>  ref-filter.c | 103 ++++++++++++++++++++++++++++++-----------------------------
>>  1 file changed, 53 insertions(+), 50 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index f9e25aea7a97e..37337b57aacf4 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1354,16 +1354,60 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
>>       return show_ref(&atom->u.refname, ref->refname);
>>  }
>>
>> +static void need_object(struct ref_array_item *ref) {
>
> Style.  The opening brace at the beginning of the function sits on
> its own line alone.

Thanks, I will fix that when we decide how to finally improve that commit.

Olga