[Patch] tty: move a definition out of switch block

[Patch] tty: move a definition out of switch block

[Amerigo Wang]

It's not good to leave a definition between 'switch'
and its first label. Move it out of the switch block.

Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Joe Peterson <joe@skyrush.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>

---
diff --git a/drivers/char/n_tty.c b/drivers/char/n_tty.c
index 2e50f4d..233aa3d 100644
--- a/drivers/char/n_tty.c
+++ b/drivers/char/n_tty.c
@@ -502,6 +502,7 @@ static void process_echoes(struct tty_struct *tty)
 			unsigned char op;
 			unsigned char *opp;
 			int no_space_left = 0;
+			unsigned int num_chars, num_bs;
 
 			/*
 			 * If the buffer byte is the start of a multi-byte
@@ -514,8 +515,6 @@ static void process_echoes(struct tty_struct *tty)
 			op = *opp;
 
 			switch (op) {
-				unsigned int num_chars, num_bs;
-
 			case ECHO_OP_ERASE_TAB:
 				if (++opp == buf_end)
 					opp -= N_TTY_BUF_SIZE;

Re: [Patch] tty: move a definition out of switch block

[Alan Cox]

On Tue, 1 Dec 2009 02:54:44 -0500
Amerigo Wang <amwang@redhat.com> wrote:

> 
> It's not good to leave a definition between 'switch'
> and its first label. Move it out of the switch block.

"Not good". On what basis is it not good to put variables local to
their scope ?

Alan

Re: [Patch] tty: move a definition out of switch block

[Cong Wang]

Alan Cox wrote:
> On Tue, 1 Dec 2009 02:54:44 -0500
> Amerigo Wang <amwang@redhat.com> wrote:
> 
>> It's not good to leave a definition between 'switch'
>> and its first label. Move it out of the switch block.
> 
> "Not good". On what basis is it not good to put variables local to
> their scope ?
> 

Hello, Alan,

Generally putting variables local in the local scope is fine, but
not for 'switch' block, to be more precisely, not for the case
where putting local variables between 'switch' and its _first_
label, IMO.

It can lead to misunderstanding easily, since 'switch'
jumps to its first label at a first glance. I know in this case
the code is _not_ wrong, but again, it's not good for reading.

Also, there's no conflicts if we put it out of 'switch' block.

Thanks.

Re: [Patch] tty: move a definition out of switch block

[Alan Cox]

> jumps to its first label at a first glance. I know in this case
> the code is _not_ wrong, but again, it's not good for reading.

So this is just your personal preference ? That seems like pointless
churn, especially given that many other people consider putting the
variables there is better than

	case foo:
	{
		Blah blah
	}
	}
}

in switches

Re: [Patch] tty: move a definition out of switch block

[Joe Peterson]

On Thu, Dec 3, 2009 at 01:36, Cong Wang <amwang@redhat.com> wrote:
> Generally putting variables local in the local scope is fine, but
> not for 'switch' block, to be more precisely, not for the case
> where putting local variables between 'switch' and its _first_
> label, IMO.

It is fine to declare variables at the start of a switch statement.
What *is* incorrect is either initializing such declared variables
(they won't get initialized) or declaring variables *after* a case
label (throws a compile error).

> It can lead to misunderstanding easily, since 'switch'
> jumps to its first label at a first glance. I know in this case
> the code is _not_ wrong, but again, it's not good for reading.

I would think the declaration inside the start of the switch block
would just indicate to the reader that the variable is local to the
switch block scope.

> Also, there's no conflicts if we put it out of 'switch' block.

But then the scope of these variables becomes larger than it need be.

-Joe

Re: [Patch] tty: move a definition out of switch block

[Cong Wang]

Alan Cox wrote:
>> jumps to its first label at a first glance. I know in this case
>> the code is _not_ wrong, but again, it's not good for reading.
> 
> So this is just your personal preference ? That seems like pointless
> churn, especially given that many other people consider putting the
> variables there is better than
> 
> 	case foo:
> 	{
> 		Blah blah
> 	}
> 	}
> }
> 
> in switches

Well, in C99 6.5.4, it has a very good example to explain this.
See this example:

switch (xxx) {
	int a = 1; //<-- not initialized
	int b; //<-- seems to be skipped, but not
	func(&a); //<-- skipped;
case 1:
	//...
	break;
case 2:
	return a; //<-- uninitialized value;
}


So why not just:

int a = 1, b;
switch (xxx) {
case 1:
	// blah blah
}

? A first galance will know everything, no need to guess if
'switch' skips it or not.

Re: [Patch] tty: move a definition out of switch block

[Joe Peterson]

On Tue, Dec 8, 2009 at 03:10, Cong Wang <amwang@redhat.com> wrote:
> Well, in C99 6.5.4, it has a very good example to explain this.
> See this example:
>
> switch (xxx) {
>        int a = 1; //<-- not initialized
>        int b; //<-- seems to be skipped, but not
>        func(&a); //<-- skipped;
> case 1:
>        //...
>        break;
> case 2:
>        return a; //<-- uninitialized value;
> }

I agree there are some strange semantics within switch statements
regarding what happens to lines before the first "case", but nothing
about declaring variables there is illegal, and the behavior is
well-defined.  Yes, statements and initializers get skipped, but the
variables are still declared.  Neither "int a" nor "int b" above are
skipped or even "seem skipped"; it is only that the initialization of
a is not performed.  Declarations (that to not initialize) are just
declarations; they do not get "executed".  Putting them at the start
of a switch block simple shows that they exist in that scope.

> So why not just:
>
> int a = 1, b;
> switch (xxx) {
> case 1:
>        // blah blah
> }
>
> ? A first galance will know everything, no need to guess if
> 'switch' skips it or not.

Main reason: variables is used only within scope of the switch
statement.  Sure, it could be moved outside, but I'm not convinced
this is vital or proper or even more clear.  It would be a bug to try
to initialize such a variable or to try to execute statements at the
start of the switch block, but this is not being done.

-Joe