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

[Оля Тележная]

2018-01-30 23:49 GMT+03:00 Junio C Hamano :
> Оля Тележная   writes:
>
>>> 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,...
>
> That sounds as if you are complaining "I wanted to get rid of goto
> and you tell me not to do so???", but read what I showed above again
> and notice that it is also getting rid of "goto".

No, I am not complaining. I tried to explain why I did everything that
way. Sorry if it was not clear enough.

>
> The main difference from your version is that the original function
> is still kept as a single unit of work, instead of two.

And I am not sure that it is good, the function is too big and it
actually does so many different separate pieces. If it is possible to
shorten long function by getting some separate logic (that's our case,
we do not request object until that final goto statement), I think
it's good idea and we need to do so and simplify future reading. But,
if you do not agree with this fact, please explain your position in
detail, and I will change that place as you want.

Thanks.

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

[Junio C Hamano]

Оля Тележная   writes:

>> 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,...

That sounds as if you are complaining "I wanted to get rid of goto
and you tell me not to do so???", but read what I showed above again
and notice that it is also getting rid of "goto".

The main difference from your version is that the original function
is still kept as a single unit of work, instead of two.

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

[Оля Тележная]

2018-01-26 23:19 GMT+03:00 Junio C Hamano :
> Olga Telezhnaya  writes:
>
>> Get rid of goto command in ref-filter for better readability.
>>
>> Signed-off-by: Olga Telezhnaia 
>> Mentored-by: Christian Couder 
>> Mentored by: Jeff King 
>> ---
>
> 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?

>
> 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.

>
> 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).

>
>>  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(>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

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

[Junio C Hamano]

Olga Telezhnaya  writes:

> Get rid of goto command in ref-filter for better readability.
>
> Signed-off-by: Olga Telezhnaia 
> Mentored-by: Christian Couder 
> Mentored by: Jeff King 
> ---

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.

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.

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.

>  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(>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.

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

[Olga Telezhnaya]

Get rid of goto command in ref-filter for better readability.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 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(>u.refname, ref->refname);
 }
 
+static void need_object(struct ref_array_item *ref) {
+   struct object *obj;
+   const struct object_id *tagged;
+   unsigned long size;
+   int eaten;
+   void *buf = get_obj(>objectname, , , );
+   if (!buf)
+   die(_("missing object %s for %s"),
+   oid_to_hex(>objectname), ref->refname);
+   if (!obj)
+   die(_("parse_object_buffer failed on %s for %s"),
+   oid_to_hex(>objectname), ref->refname);
+
+   grab_values(ref->value, 0, obj, buf, size);
+   if (!eaten)
+   free(buf);
+
+   /*
+* If there is no atom that wants to know about tagged
+* object, we are done.
+*/
+   if (!need_tagged || (obj->type != OBJ_TAG))
+   return;
+
+   /*
+* If it is a tag object, see if we use a value that derefs
+* the object, and if we do grab the object it refers to.
+*/
+   tagged = &((struct tag *)obj)->tagged->oid;
+
+   /*
+* NEEDSWORK: This derefs tag only once, which
+* is good to deal with chains of trust, but
+* is not consistent with what deref_tag() does
+* which peels the onion to the core.
+*/
+   buf = get_obj(tagged, , , );
+   if (!buf)
+   die(_("missing object %s for %s"),
+   oid_to_hex(tagged), ref->refname);
+   if (!obj)
+   die(_("parse_object_buffer failed on %s for %s"),
+   oid_to_hex(tagged), ref->refname);
+   grab_values(ref->value, 1, obj, buf, size);
+   if (!eaten)
+   free(buf);
+}
+
 /*
  * Parse the object referred by ref, and grab needed value.
  */
 static void populate_value(struct ref_array_item *ref)
 {
-   void *buf;
-   struct object *obj;
-   int eaten, i;
-   unsigned long size;
-   const struct object_id *tagged;
+   int i;
 
ref->value = xcalloc(used_atom_cnt, sizeof(struct atom_value));
 
@@ -1477,53 +1521,12 @@ static void populate_value(struct ref_array_item *ref)
 
for (i = 0; i < used_atom_cnt; i++) {
struct atom_value *v = >value[i];
-   if (v->s == NULL)
-   goto need_obj;
+   if (v->s == NULL) {
+   need_object(ref);
+   break;
+   }
}
return;
-
- need_obj:
-   buf = get_obj(>objectname, , , );
-   if (!buf)
-   die(_("missing object %s for %s"),
-   oid_to_hex(>objectname), ref->refname);
-   if (!obj)
-   die(_("parse_object_buffer failed on %s for %s"),
-   oid_to_hex(>objectname), ref->refname);
-
-   grab_values(ref->value, 0, obj, buf, size);
-   if (!eaten)
-   free(buf);
-
-   /*
-* If there is no atom that wants to know about tagged
-* object, we are done.
-*/
-   if (!need_tagged || (obj->type != OBJ_TAG))
-   return;
-
-   /*
-* If it is a tag object, see if we use a value that derefs
-* the object, and if we do grab the object it refers to.
-*/
-   tagged = &((struct tag *)obj)->tagged->oid;
-
-   /*
-* NEEDSWORK: This derefs tag only once, which
-* is good to deal with chains of trust, but
-* is not consistent with what deref_tag() does
-* which peels the onion to the core.
-*/
-   buf = get_obj(tagged, , , );
-   if (!buf)
-   die(_("missing object %s for %s"),
-   oid_to_hex(tagged), ref->refname);
-   if (!obj)
-   die(_("parse_object_buffer failed on %s for %s"),
-   oid_to_hex(tagged), ref->refname);
-   grab_values(ref->value, 1, obj, buf, size);
-   if (!eaten)
-   free(buf);
 }
 
 /*

--
https://github.com/git/git/pull/452