[2/5] cifs: remove unneeded variable initialization in cifs_reconnect_tcon

Commit Message

Steve French April 5, 2011, 5:06 p.m. UTC

This is fine, but what about this alternative (initializing variables
can make code easier to read, and given dumb compiler warnings
sometimes better to leave in variable initialization ...):

stevef@stevef-laptop:~/cifs-2.6$ git diff -a


On Thu, Mar 31, 2011 at 4:36 PM, Jeff Layton <[e-mail]> wrote:
> Reported-and-acked-by: David Howells <[e-mail]>
> Signed-off-by: Jeff Layton <[e-mail]>
> ---
>  fs/cifs/cifssmb.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 1622978..4f5c6d0 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -106,7 +106,7 @@ static void mark_open_files_invalid(struct cifs_tcon *pTcon)
>  static int
>  cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
>  {
> -       int rc = 0;
> +       int rc;
>        struct cifs_ses *ses;
>        struct TCP_Server_Info *server;
>        struct nls_table *nls_codepage;
> --
> 1.7.4
>
>

Comments

Jeff Layton April 5, 2011, 5:17 p.m. UTC | #1

On Tue, 5 Apr 2011 12:06:59 -0500
Steve French <[e-mail]> wrote:

> This is fine, but what about this alternative (initializing variables
> can make code easier to read, and given dumb compiler warnings
> sometimes better to leave in variable initialization ...):
> 
> stevef@stevef-laptop:~/cifs-2.6$ git diff -a
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 92e33d4..e4993a2 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -117,7 +117,7 @@ cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
>          * calling routine
>          */
>         if (!tcon)
> -               return 0;
> +               return rc;
> 
>         ses = tcon->ses;
>         server = ses->server;
> 
> 

So instead of eliminating a useless variable initialization, we instead
manufacture a use for it? That seems sort of counter-productive, no?

Steve French April 5, 2011, 5:20 p.m. UTC | #2

On Tue, Apr 5, 2011 at 12:17 PM, Jeff Layton <[e-mail]> wrote:
> On Tue, 5 Apr 2011 12:06:59 -0500
> Steve French <[e-mail]> wrote:
>
>> This is fine, but what about this alternative (initializing variables
>> can make code easier to read, and given dumb compiler warnings
>> sometimes better to leave in variable initialization ...):
>>
>> stevef@stevef-laptop:~/cifs-2.6$ git diff -a
>> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
>> index 92e33d4..e4993a2 100644
>> --- a/fs/cifs/cifssmb.c
>> +++ b/fs/cifs/cifssmb.c
>> @@ -117,7 +117,7 @@ cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
>>          * calling routine
>>          */
>>         if (!tcon)
>> -               return 0;
>> +               return rc;
>>
>>         ses = tcon->ses;
>>         server = ses->server;
>>
>>
>
> So instead of eliminating a useless variable initialization, we instead
> manufacture a use for it? That seems sort of counter-productive, no?

since we do "return rc" later in the function, why wouldn't we do the
same earlier
in the function for consistency? With rc initialized it is a bit easier to read.

Jeff Layton April 5, 2011, 5:28 p.m. UTC | #3

On Tue, 5 Apr 2011 12:20:52 -0500
Steve French <[e-mail]> wrote:

> On Tue, Apr 5, 2011 at 12:17 PM, Jeff Layton <[e-mail]> wrote:
> > On Tue, 5 Apr 2011 12:06:59 -0500
> > Steve French <[e-mail]> wrote:
> >
> >> This is fine, but what about this alternative (initializing variables
> >> can make code easier to read, and given dumb compiler warnings
> >> sometimes better to leave in variable initialization ...):
> >>
> >> stevef@stevef-laptop:~/cifs-2.6$ git diff -a
> >> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> >> index 92e33d4..e4993a2 100644
> >> --- a/fs/cifs/cifssmb.c
> >> +++ b/fs/cifs/cifssmb.c
> >> @@ -117,7 +117,7 @@ cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
> >>          * calling routine
> >>          */
> >>         if (!tcon)
> >> -               return 0;
> >> +               return rc;
> >>
> >>         ses = tcon->ses;
> >>         server = ses->server;
> >>
> >>
> >
> > So instead of eliminating a useless variable initialization, we instead
> > manufacture a use for it? That seems sort of counter-productive, no?
> 
> since we do "return rc" later in the function, why wouldn't we do the
> same earlier
> in the function for consistency? With rc initialized it is a bit easier to read.
> 

In those places, we're returning rc because we called a function and
stored the return code from it in rc. You certainly could "return rc"
solely (and possibly use a "goto out" and have a single exit point for
the function). The above patch doesn't do that though, it only changes
the first spot where it does a "return 0".

If you want to make that change then you probably ought to change the
other places in the function for consistency. Again though, it seems
pointless to me. Leaving it unintialized may help the compiler catch
places where we "return rc" without setting it intentionally.

Patch

diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 92e33d4..e4993a2 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -117,7 +117,7 @@  cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
         * calling routine
         */
        if (!tcon)
-               return 0;
+               return rc;

        ses = tcon->ses;
        server = ses->server;