threads / discuss / 24431

remove duplicate code and not needed break statement

Subject: remove duplicate code and not needed break statement

## tl;dr

9 messages between Jul 18, 2010 and Jul 19, 2010.

replies: 8people: 4as markdown or json

Ralf Thielow· Jul 18, 2010, 17:49 UTC · lore
Remove duplicate code and not needed break statement.

On switch statements you can use multiple cases for one statement and on a goto statement you not need a break.

>From 5b3e7c8f8b81a295b5c58534be250f5a818ccc64 Mon Sep 17 00:00:00 2001
From: Ralf Thielow <ralf.thielow@googlemail.com>
Date: Sun, 18 Jul 2010 18:48:58 +0200
Subject: [PATCH] remove duplicate code and not needed break statement
---
 server-info.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/server-info.c b/server-info.c
index 4098ca2..9ec744e 100644
--- a/server-info.c
+++ b/server-info.c
@@ -113,11 +113,8 @@ static int read_pack_info_file(const char *infofile)
 				goto out_stale;
 			break;
 		case 'D': /* we used to emit D but that was misguided. */
-			goto out_stale;
-			break;
 		case 'T': /* we used to emit T but nobody uses it. */
 			goto out_stale;
-			break;
 		default:
 			error("unrecognized: %s", line);
 			break;
Valeo de Vries· Jul 18, 2010, 17:57 UTC · re: Ralf Thielow · lore

Re: remove duplicate code and not needed break statement

On 18 July 2010 18:49, Ralf Thielow <ralf.thielow@googlemail.com> wrote:
Show 21 quoted lines
> Remove duplicate code and not needed break statement.
>
> On switch statements you can use multiple cases for one
> statement and on a goto statement you not need a break.
>
> From 5b3e7c8f8b81a295b5c58534be250f5a818ccc64 Mon Sep 17 00:00:00 2001
> From: Ralf Thielow <ralf.thielow@googlemail.com>
> Date: Sun, 18 Jul 2010 18:48:58 +0200
> Subject: [PATCH] remove duplicate code and not needed break statement
>
> ---
>  server-info.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
>
> diff --git a/server-info.c b/server-info.c
> index 4098ca2..9ec744e 100644
> --- a/server-info.c
> +++ b/server-info.c
> @@ -113,11 +113,8 @@ static int read_pack_info_file(const char *infofile)
>                                goto out_stale;
>                        break;
You seem to have missed another, or am I just lacking context? ;)
Valeo
Ralf Thielow· Jul 18, 2010, 18:29 UTC · re: Valeo de Vries · lore

Re: remove duplicate code and not needed break statement

2010/7/18 Valeo de Vries <valeo@valeo.co.cc>:
Show 27 quoted lines
> On 18 July 2010 18:49, Ralf Thielow <ralf.thielow@googlemail.com> wrote:
>> Remove duplicate code and not needed break statement.
>>
>> On switch statements you can use multiple cases for one
>> statement and on a goto statement you not need a break.
>>
>> From 5b3e7c8f8b81a295b5c58534be250f5a818ccc64 Mon Sep 17 00:00:00 2001
>> From: Ralf Thielow <ralf.thielow@googlemail.com>
>> Date: Sun, 18 Jul 2010 18:48:58 +0200
>> Subject: [PATCH] remove duplicate code and not needed break statement
>>
>> ---
>>  server-info.c |    3 ---
>>  1 files changed, 0 insertions(+), 3 deletions(-)
>>
>> diff --git a/server-info.c b/server-info.c
>> index 4098ca2..9ec744e 100644
>> --- a/server-info.c
>> +++ b/server-info.c
>> @@ -113,11 +113,8 @@ static int read_pack_info_file(const char *infofile)
>>                                goto out_stale;
>>                        break;
>
> You seem to have missed another, or am I just lacking context? ;)
>
> Valeo
>

There were 2 cases on that switch statement which do the same thing. in case 'P' there is an if-statment before.

Ralf
Jonathan Nieder· Jul 18, 2010, 18:47 UTC · re: Ralf Thielow · lore

Re: remove duplicate code and not needed break statement

Hi Ralf!
Ralf Thielow wrote:
> Remove duplicate code and not needed break statement.

This looks like a reasonable change, with the nice effect of getting read_pack_info_file() to almost fit in one screen.

The commit message is not super clear.  I would say:
 Subject: update-server-info: Shorten read_pack_info_file()
 The correct responses to a D and a T line in .git/objects/info/packs
 are the same, so combine their case arms.  In both cases we already
 ‘goto’ out of the switch so while at it, remove a redundant ‘break’
 to avoid yet another line of code.
i.e., impact in the subject line, details in the commit message body.

It is missing a sign-off. See Documentation/SubmittingPatches for what that means.

Hope that helps, Jonathan

Ralf Thielow· Jul 18, 2010, 18:56 UTC · re: Jonathan Nieder · lore

Re: remove duplicate code and not needed break statement

2010/7/18 Jonathan Nieder <jrnieder@gmail.com>:
Show 26 quoted lines
> Hi Ralf!
>
> Ralf Thielow wrote:
>
>> Remove duplicate code and not needed break statement.
>
> This looks like a reasonable change, with the nice effect of getting
> read_pack_info_file() to almost fit in one screen.
>
> The commit message is not super clear.  I would say:
>
>  Subject: update-server-info: Shorten read_pack_info_file()
>
>  The correct responses to a D and a T line in .git/objects/info/packs
>  are the same, so combine their case arms.  In both cases we already
>  ‘goto’ out of the switch so while at it, remove a redundant ‘break’
>  to avoid yet another line of code.
>
> i.e., impact in the subject line, details in the commit message body.
>
> It is missing a sign-off.  See Documentation/SubmittingPatches for
> what that means.
>
> Hope that helps,
> Jonathan
>

Thank you, this is my first patch on a mailing list using git, you see. In future I do it better and read the whole documentation before. :)

Ralf
Erik Faye-Lund· Jul 19, 2010, 15:43 UTC · re: Ralf Thielow · lore

Re: remove duplicate code and not needed break statement

On Sun, Jul 18, 2010 at 7:49 PM, Ralf Thielow <ralf.thielow@googlemail.com> wrote:

Show 25 quoted lines
> Remove duplicate code and not needed break statement.
>
> On switch statements you can use multiple cases for one
> statement and on a goto statement you not need a break.
>
> From 5b3e7c8f8b81a295b5c58534be250f5a818ccc64 Mon Sep 17 00:00:00 2001
> From: Ralf Thielow <ralf.thielow@googlemail.com>
> Date: Sun, 18 Jul 2010 18:48:58 +0200
> Subject: [PATCH] remove duplicate code and not needed break statement
>
> ---
>  server-info.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
>
> diff --git a/server-info.c b/server-info.c
> index 4098ca2..9ec744e 100644
> --- a/server-info.c
> +++ b/server-info.c
> @@ -113,11 +113,8 @@ static int read_pack_info_file(const char *infofile)
>                                goto out_stale;
>                        break;
>                case 'D': /* we used to emit D but that was misguided. */
> -                       goto out_stale;
> -                       break;
>                case 'T': /* we used to emit T but nobody uses it. */

We have a tendency of adding a comment pointing out fall through between case-statements. Perhaps you should add one? 'git grep -i fall.\\?through' shows you some of the places we've got it now...

-- 
Erik "kusma" Faye-Lund
Jonathan Nieder· Jul 19, 2010, 16:06 UTC · re: Erik Faye-Lund · lore

Re: remove duplicate code and not needed break statement

Hi Erik,
Erik Faye-Lund wrote:
> On Sun, Jul 18, 2010 at 7:49 PM, Ralf Thielow
Show 7 quoted lines
>>                case 'D': /* we used to emit D but that was misguided. */
>> -                       goto out_stale;
>> -                       break;
>>                case 'T': /* we used to emit T but nobody uses it. */
>
> We have a tendency of adding a comment pointing out fall through
> between case-statements. Perhaps you should add one?

I think in this case that would not be appropriate. It is the difference between:

	case 1:
	case 2:
	case 3:
		puts("one to three");
		break;
and
	case 1:
		puts("one");
		/* fall through */
	case 2:
	case 3:
		puts("one to three");
		break;

I admit, I am surprised to see multiple suggestions for improvements to details of the patch. I guess I should say, except for the log message, it is

  Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

i.e., I don’t see any low-hanging fruit for improving it without actually taking a deeper look at the surrounding code. Ralf, can we have your sign-off? That way maybe it could be applied and people could suggest patches on top.

Still, thanks for looking it over.
Jonathan
Ralf Thielow· Jul 19, 2010, 16:26 UTC · re: Jonathan Nieder · lore

Re: remove duplicate code and not needed break statement

From 5b3e7c8f8b81a295b5c58534be250f5a818ccc64 Mon Sep 17 00:00:00 2001
From: Ralf Thielow <ralf.thielow@googlemail.com>
Date: Sun, 18 Jul 2010 18:48:58 +0200
Subject: [PATCH] update-server-info: Shorten read_pack_info_file()

The correct responses to a D and a T line in .git/objects/info/packs are the same, so combine their case arms. In both cases we already ‘goto’ out of the switch so while at it, remove a redundant ‘break’ to avoid yet another line of code.

Signed-off-by: Ralf Thielow <ralf.thielow@googlemail.com>
Reviewed-by: Jonathan Nieder <jrnieder <at> gmail.com>
---
 server-info.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/server-info.c b/server-info.c
index 4098ca2..9ec744e 100644
--- a/server-info.c
+++ b/server-info.c
@@ -113,11 +113,8 @@ static int read_pack_info_file(const char *infofile)
 				goto out_stale;
 			break;
 		case 'D': /* we used to emit D but that was misguided. */
-			goto out_stale;
-			break;
 		case 'T': /* we used to emit T but nobody uses it. */
 			goto out_stale;
-			break;
 		default:
 			error("unrecognized: %s", line);
 			break;

-- 
1.7.0.4
Ralf Thielow· Jul 18, 2010, 18:19 UTC · lore

Re: remove duplicate code and not needed break statement

There were 2 cases on that switch statement which do the same thing. in case 'P' there is an if-statment before.

Ralf
Show 29 quoted lines
> 2010/7/18 Valeo de Vries <valeo@valeo.co.cc>:
>> On 18 July 2010 18:49, Ralf Thielow <ralf.thielow@googlemail.com> wrote:
>>> Remove duplicate code and not needed break statement.
>>>
>>> On switch statements you can use multiple cases for one
>>> statement and on a goto statement you not need a break.
>>>
>>> From 5b3e7c8f8b81a295b5c58534be250f5a818ccc64 Mon Sep 17 00:00:00 2001
>>> From: Ralf Thielow <ralf.thielow@googlemail.com>
>>> Date: Sun, 18 Jul 2010 18:48:58 +0200
>>> Subject: [PATCH] remove duplicate code and not needed break statement
>>>
>>> ---
>>>  server-info.c |    3 ---
>>>  1 files changed, 0 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/server-info.c b/server-info.c
>>> index 4098ca2..9ec744e 100644
>>> --- a/server-info.c
>>> +++ b/server-info.c
>>> @@ -113,11 +113,8 @@ static int read_pack_info_file(const char *infofile)
>>>                                goto out_stale;
>>>                        break;
>>
>> You seem to have missed another, or am I just lacking context? ;)
>>
>> Valeo
>>
>

← back to recent threads