[patch 1/8] isdn_bsdcomp.c - vfree() checking cleanups

[patch 1/8] isdn_bsdcomp.c - vfree() checking cleanups

[domen]

isdn_bsdcomp.c vfree() checking cleanups.

Signed-off by: James Lamanna <jlamanna@gmail.com>
Signed-off-by: Domen Puncer <domen@coderock.org>
---


 kj-domen/drivers/isdn/i4l/isdn_bsdcomp.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff -puN drivers/isdn/i4l/isdn_bsdcomp.c~vfree-drivers_isdn_i4l_isdn_bsdcomp drivers/isdn/i4l/isdn_bsdcomp.c
--- kj/drivers/isdn/i4l/isdn_bsdcomp.c~vfree-drivers_isdn_i4l_isdn_bsdcomp	2005-03-05 16:10:31.000000000 +0100
+++ kj-domen/drivers/isdn/i4l/isdn_bsdcomp.c	2005-03-05 16:10:31.000000000 +0100
@@ -283,18 +283,14 @@ static void bsd_free (void *state)
 		/*
 		 * Release the dictionary
 		 */
-		if (db->dict) {
-			vfree (db->dict);
-			db->dict = NULL;
-		}
+		vfree (db->dict);
+		db->dict = NULL;
 
 		/*
 		 * Release the string buffer
 		 */
-		if (db->lens) {
-			vfree (db->lens);
-			db->lens = NULL;
-		}
+		vfree (db->lens);
+		db->lens = NULL;
 
 		/*
 		 * Finally release the structure itself.
_

Re: [patch 1/8] isdn_bsdcomp.c - vfree() checking cleanups

[Ralph Corderoy]

Hi Domen,

> -		if (db->dict) {
> -			vfree (db->dict);
> -			db->dict = NULL;
> -		}
> +		vfree (db->dict);
> +		db->dict = NULL;

Is it really worth always calling vfree() which calls __vunmap() before
db->dict is determined to be NULL in order to turn three lines into two?
Plus the write to db->dict which might otherwise not be needed.  The old
code was clear, clean, and fast, no?

Cheers,


Ralph.

Re: [patch 1/8] isdn_bsdcomp.c - vfree() checking cleanups

[Domen Puncer]

On 07/03/05 00:07 +0000, Ralph Corderoy wrote:
> 
> Hi Domen,
> 
> > -		if (db->dict) {
> > -			vfree (db->dict);
> > -			db->dict = NULL;
> > -		}
> > +		vfree (db->dict);
> > +		db->dict = NULL;
> 
> Is it really worth always calling vfree() which calls __vunmap() before
> db->dict is determined to be NULL in order to turn three lines into two?

Four lines into two :-)

> Plus the write to db->dict which might otherwise not be needed.  The old
> code was clear, clean, and fast, no?

Shorter and more readable code is always better, right? And speed really
doesn't seem to be an issue here.


	Domen

Re: [patch 1/8] isdn_bsdcomp.c - vfree() checking cleanups

[Karsten Keil]

On Mon, Mar 07, 2005 at 01:21:33AM +0100, Domen Puncer wrote:
> On 07/03/05 00:07 +0000, Ralph Corderoy wrote:
> > 
> > Hi Domen,
> > 
> > > -		if (db->dict) {
> > > -			vfree (db->dict);
> > > -			db->dict = NULL;
> > > -		}
> > > +		vfree (db->dict);
> > > +		db->dict = NULL;
> > 
> > Is it really worth always calling vfree() which calls __vunmap() before
> > db->dict is determined to be NULL in order to turn three lines into two?
> 
> Four lines into two :-)
> 
> > Plus the write to db->dict which might otherwise not be needed.  The old
> > code was clear, clean, and fast, no?
> 
> Shorter and more readable code is always better, right? And speed really
> doesn't seem to be an issue here.
> 

I also prefer the old code, since it make clear, that you must be careful
here, since the function can be called with already freed db->dict, and for
me this version is not better readable as the old one.

-- 
Karsten Keil
SuSE Labs
ISDN development

Re: [patch 1/8] isdn_bsdcomp.c - vfree() checking cleanups

[Ralph Corderoy]

Hi Domen,

> On 07/03/05 00:07 +0000, Ralph Corderoy wrote:
> > > -		if (db->dict) {
> > > -			vfree (db->dict);
> > > -			db->dict = NULL;
> > > -		}
> > > +		vfree (db->dict);
> > > +		db->dict = NULL;
> > 
> > Is it really worth always calling vfree() which calls __vunmap()
> > before db->dict is determined to be NULL in order to turn three
> > lines into two?
> 
> Four lines into two :-)
> 
> > Plus the write to db->dict which might otherwise not be needed.  The
> > old code was clear, clean, and fast, no?
> 
> Shorter and more readable code is always better, right?

No.  Let me try and persuade you.

There's three cases.

    1.  foo will always be NULL at the line in question so no need to
    `vfree(foo); foo = NULL;'.

    2.  foo will never be NULL at the line in question so `vfree(foo);
    foo = NULL;' is mandatory.

    3.  foo will sometimes be NULL, sometimes not.

In that third case, seeing

    if (foo) {
        vfree(foo);
        foo = NULL;
    }

tells the reader that we're dealing with the `foo *maybe* NULL' case
whereas

    vfree(foo);
    foo = NULL;

*suggests* the `foo is never NULL' case.  The reader has to remember
that vfree(NULL) is valid and bear that in mind.

If the reader is about to modify the preceeding lines, knowing the foo
may sometimes be NULL helps.  So I prefer the longer code here because
it better shows the intent of the coder and is more telling about the
state of db->dict.

If you're really keen to save lines, please axe the superfluous
three-line comments.  We read C here, they're unnecessary.

>  		/*
>  		 * Release the dictionary
>  		 */
> -		if (db->dict) {
> -			vfree (db->dict);
> -			db->dict = NULL;
> -		}
> +		vfree (db->dict);
> +		db->dict = NULL;
>  
>  		/*
>  		 * Release the string buffer
>  		 */
> -		if (db->lens) {
> -			vfree (db->lens);
> -			db->lens = NULL;
> -		}
> +		vfree (db->lens);
> +		db->lens = NULL;
>  
>  		/*
>  		 * Finally release the structure itself.

Cheers,


Ralph.