Re: [PATCH] CodingStyle fixes for drm_agpsupport

Re: [PATCH] CodingStyle fixes for drm_agpsupport

[Ed Cogburn]

Larry McVoy wrote:
> On Mon, Aug 11, 2003 at 01:53:17PM -0400, Jeff Garzik wrote:
> 
>>Larry McVoy wrote:
>>are function calls at a 10-nanosecond glance.  Also, having two styles 
>>of 'if' formatting in your example just screams "inconsistent" to me :)
> 
> 
> It is inconsistent, on purpose.  It's essentially like perl's
> 
> 	return unless pointer;
> 
> which is a oneliner, almost like an assert().
> 
> Maybe this will help: I insist on braces on anything with indentation so
> that I can scan them more quickly.  If I gave you a choice between
> 
> 	if (!pointer) {
> 		return (whatever);
> 	}
> 
> 	if (!pointer) return (whatever);
> 
> which one will you type more often?  I actually don't care which you use,
> I prefer the shorter one because I don't measure my self worth in lines 
> of code generated, I tend to favor lines of code deleted :)  But either
> one is fine, I tend to use the first one if it has been a problem area
> and I'm likely to come back and shove in some debugging.


I prefer keeping the conditional statement separate from the condition, but 
either way works.  One thing I've noticed though is that one line if statements 
are difficult to debug in a debugger because there is no way to tell by watching 
the current debug line whether the conditional statement was executed or not. 
For that reason I use a two line if.  Of course, rumor has it that real 
programmers don't use debuggers....  :)

I would rather use the extra lines for two line if statements, then make up for 
that used space by avoiding unnecessary braces.

Re: [PATCH] CodingStyle fixes for drm_agpsupport

[Gene Heskett]

On Thursday 14 August 2003 11:28, Larry McVoy wrote:
>> >which can look through the formatting changes and give you the
>> > semantic knowledge that you want.  We'd love to see how it is
>> > done and then do it in BitKeeper :)
>>
>> <troll>What?!  And _copy_ someone else's hard work?!</troll>
>> *cough*
>
><sarcasm>
>Well, my eyes have been opened by all the thoughtful and insightful
>arguments put forth on this list about the ethics of reverse
> engineering. </sarcasm>

Tee hee, carefull there Larry, there be dragons there. :)

-- 
Cheers, Gene
AMD K6-III@500mhz 320M
Athlon1600XP@1400mhz  512M
99.27% setiathome rank, not too shabby for a WV hillbilly
Yahoo.com attornies please note, additions to this message
by Gene Heskett are:
Copyright 2003 by Maurice Eugene Heskett, all rights reserved.

Re: [PATCH] CodingStyle fixes for drm_agpsupport

[Larry McVoy]

> >which can look through the formatting changes and give you the semantic
> >knowledge that you want.  We'd love to see how it is done and then do
> >it in BitKeeper :)
> 
> <troll>What?!  And _copy_ someone else's hard work?!</troll> *cough* 

<sarcasm>
Well, my eyes have been opened by all the thoughtful and insightful
arguments put forth on this list about the ethics of reverse engineering.
</sarcasm>
-- 
---
Larry McVoy              lm at bitmover.com          http://www.bitmover.com/lm

Re: [PATCH] CodingStyle fixes for drm_agpsupport

[Eli Carter]

Larry McVoy wrote:
> On Thu, Aug 14, 2003 at 09:21:44AM -0500, Eli Carter wrote:
> 
>>>That ought to be balanced with "don't screw up the revision history, people
>>>use it".  It's one thing to reformat code that is unreadable, for the most
>>>part this code didn't come close to unreadable.
>>
>>Devil's advocate:
>>Then perhaps the (revision control) tool is getting in the way of doing 
>>the job and should be fixed?  :)
>>Perhaps being able to flag a changeset as a 'formatting change', and 
>>have the option to hide it or make it 'transparent' in some fashion? 
>>Hmm... "Annotate only the changes that relate to feature X."...
>>Oh, and a complete AI with that if you don't mind. ;)
>>
>>But you've probably already thought about all this...
> 
> 
> Indeed I have. 

Figured. :)

 > And there is a reason that we have a policy at BitMover
> where "formatting changes" are prohibited and we make people redo their
> changesets until they get them right.

Ah yes, I do see the value of enforcing a coding style from the get-go.

> In other words, you are welcome to write a revision control system
> which can look through the formatting changes and give you the semantic
> knowledge that you want.  We'd love to see how it is done and then do
> it in BitKeeper :)

<troll>What?!  And _copy_ someone else's hard work?!</troll> *cough* 
(Sorry, couldn't resist. ;) )

Eli
--------------------. "If it ain't broke now,
Eli Carter           \                  it will be soon." -- crypto-gram
eli.carter(a)inet.com `-------------------------------------------------

Re: [PATCH] CodingStyle fixes for drm_agpsupport

[Larry McVoy]

On Thu, Aug 14, 2003 at 09:21:44AM -0500, Eli Carter wrote:
> >That ought to be balanced with "don't screw up the revision history, people
> >use it".  It's one thing to reformat code that is unreadable, for the most
> >part this code didn't come close to unreadable.
> 
> Devil's advocate:
> Then perhaps the (revision control) tool is getting in the way of doing 
> the job and should be fixed?  :)
> Perhaps being able to flag a changeset as a 'formatting change', and 
> have the option to hide it or make it 'transparent' in some fashion? 
> Hmm... "Annotate only the changes that relate to feature X."...
> Oh, and a complete AI with that if you don't mind. ;)
> 
> But you've probably already thought about all this...

Indeed I have.  And there is a reason that we have a policy at BitMover
where "formatting changes" are prohibited and we make people redo their
changesets until they get them right.

In other words, you are welcome to write a revision control system
which can look through the formatting changes and give you the semantic
knowledge that you want.  We'd love to see how it is done and then do
it in BitKeeper :)
-- 
---
Larry McVoy              lm at bitmover.com          http://www.bitmover.com/lm

Re: [PATCH] CodingStyle fixes for drm_agpsupport

[Eli Carter]

Larry McVoy wrote:
> On Mon, Aug 11, 2003 at 12:58:44PM -0400, Jeff Garzik wrote:
> 
>>Larry McVoy wrote:
>>
>>>A few comments on why I don't like this patch:
>>>   1) It's a formatting only patch.  That screws over people who are using
>>>      BK for debugging, now when I double click on these changes I'll get
>>>      to your cleanup patch, not the patch that was the last substantive
>>>      change.
>>
>>This is true, but at the same time, in Linux CodingStyle patches 
>>culturally acceptable.  I think the general logic is just "don't go 
>>overboard; reformat a tiny fragment at a time."
> 
> 
> That ought to be balanced with "don't screw up the revision history, people
> use it".  It's one thing to reformat code that is unreadable, for the most
> part this code didn't come close to unreadable.

Devil's advocate:
Then perhaps the (revision control) tool is getting in the way of doing 
the job and should be fixed?  :)
Perhaps being able to flag a changeset as a 'formatting change', and 
have the option to hide it or make it 'transparent' in some fashion? 
Hmm... "Annotate only the changes that relate to feature X."...
Oh, and a complete AI with that if you don't mind. ;)

But you've probably already thought about all this...

Eli
--------------------. "If it ain't broke now,
Eli Carter           \                  it will be soon." -- crypto-gram
eli.carter(a)inet.com `-------------------------------------------------

Re: [PATCH] CodingStyle fixes for drm_agpsupport

[Jamie Lokier]

Werner Almesberger wrote:
> <stmt> ::= if (<expression>) <stmt>
> <stmt> ::= { <newline> <stmts> } <newline>
> <stmt> ::= <expression> ; <newline>
> etc.
> 
> Perfectly consistent. ("Always end a statement with a newline.")

So you agree with Javascript that the semicolons aren't necessary :)

-- Jamie

Re: [PATCH] CodingStyle fixes for drm_agpsupport

[Werner Almesberger]

Jeff Garzik wrote:
> Also, having two styles 
> of 'if' formatting in your example just screams "inconsistent" to me :)

Naw, it's only one style:

<stmt> ::= if (<expression>) <stmt>
<stmt> ::= { <newline> <stmts> } <newline>
<stmt> ::= <expression> ; <newline>
etc.

Perfectly consistent. ("Always end a statement with a newline.")

> Ug.  The first and last 'if' need spreading out away from the big fat 
> block,

Why waste a perfectly good punch card on that second line ? :-)

My own formatting rules for "if" go about like this:

 1) if "if" and the statement fit on a single line, put them accordingly
    ("vertical space is precious")

 2) if the line needs to wrap (I wrap at the 79th column), always put
    the statement on a separate line, fully indented ("wrap at the
    highest hierarchical level")

 3) likewise for "else" and statement. Here, 2) doesn't apply for kernel
    code, which uses a full tab for indentation - in my user-space code,
    I use only four spaces, so sometimes, I get
    else
        stuff_that_just_happens_to_be_too_long_by_one_silly_little_character();

 4) if the statement is a block, and there is an "else" branch with a
    single statement, invert the condition ("don't hide the fine print")

Since I'm the only sentient being in the universe, and all the rest
of you are just figments of my imagination, it's fairly obvious that
my style is The Right Style :-)

> and the "return (whatever)" fools your eyes into thinking they 
> are function calls at a 10-nanosecond glance. 

Yeah, I hate that too.

Larry:
> > I also make people do
> > 
> > 	if ((a <= B) || (c >= d)) {

Argl. That's one place where the precedence works beautifully.
Extra parentheses in trivial cases always make me suspect that
the author was actually trying to do something else.

- Werner

-- 
  _________________________________________________________________________
 / Werner Almesberger, Buenos Aires, Argentina         wa@almesberger.net /
/_http://www.almesberger.net/____________________________________________/

Re: [PATCH] CodingStyle fixes for drm_agpsupport

[Jeff Garzik]

Larry McVoy wrote:
> On Mon, Aug 11, 2003 at 01:53:17PM -0400, Jeff Garzik wrote:
> 
>>Larry McVoy wrote:
>>are function calls at a 10-nanosecond glance.  Also, having two styles 
>>of 'if' formatting in your example just screams "inconsistent" to me :)
> 
> 
> It is inconsistent, on purpose.  It's essentially like perl's
> 
> 	return unless pointer;
> 
> which is a oneliner, almost like an assert().

perl is a yucky language full of hacks like this... one of the reasons 
why I love it :)

So while perl's syntax sugar allows my hands to type a bit less, I'll 
often find myself following a C style and doing

	return
		unless statement;

In general, I will continue to say the 'if' test should be completely 
separately from the statement, no matter how short either are.


> Maybe this will help: I insist on braces on anything with indentation so
> that I can scan them more quickly.  If I gave you a choice between
> 
> 	if (!pointer) {
> 		return (whatever);
> 	}
> 
> 	if (!pointer) return (whatever);
> 
> which one will you type more often?  I actually don't care which you use,
> I prefer the shorter one because I don't measure my self worth in lines 
> of code generated, I tend to favor lines of code deleted :)  But either
> one is fine, I tend to use the first one if it has been a problem area
> and I'm likely to come back and shove in some debugging.
> 
> Before you say "lose the braces" try reading more code and see how much faster
> it is if all indented stuff has braces.  You whiz through it.

Unless you get more than one or two independent 'if' tests like that, 
then all those braces for a one-line test eat up wasted screen real 
estate, slowing down the person reading the code.

So, if you gave me the choice above, I would choose option C, neither :)


>>Absolutely not.  I'm cooler, so my opinion counts more.
> 
> 
> You are in North Carolina, I'm in San Francisco.  No competition, you are
> sweating like a pig :)
> 
> 
>>>Same for your eyes when you get to my age.  
>>
>>I bet when you were in school, you had to chip your homework into slate, 
>>and dinner was brontosaurus-kebob, right?
> 
> 
> Dinner?  You got dinner?  Damn, you were spoiled.

hehe :)

	Jeff

Re: [PATCH] CodingStyle fixes for drm_agpsupport

[Larry McVoy]

On Mon, Aug 11, 2003 at 01:53:17PM -0400, Jeff Garzik wrote:
> Larry McVoy wrote:
> are function calls at a 10-nanosecond glance.  Also, having two styles 
> of 'if' formatting in your example just screams "inconsistent" to me :)

It is inconsistent, on purpose.  It's essentially like perl's

	return unless pointer;

which is a oneliner, almost like an assert().

Maybe this will help: I insist on braces on anything with indentation so
that I can scan them more quickly.  If I gave you a choice between

	if (!pointer) {
		return (whatever);
	}

	if (!pointer) return (whatever);

which one will you type more often?  I actually don't care which you use,
I prefer the shorter one because I don't measure my self worth in lines 
of code generated, I tend to favor lines of code deleted :)  But either
one is fine, I tend to use the first one if it has been a problem area
and I'm likely to come back and shove in some debugging.

Before you say "lose the braces" try reading more code and see how much faster
it is if all indented stuff has braces.  You whiz through it.

> Absolutely not.  I'm cooler, so my opinion counts more.

You are in North Carolina, I'm in San Francisco.  No competition, you are
sweating like a pig :)

> >Same for your eyes when you get to my age.  
> 
> I bet when you were in school, you had to chip your homework into slate, 
> and dinner was brontosaurus-kebob, right?

Dinner?  You got dinner?  Damn, you were spoiled.
-- 
---
Larry McVoy              lm at bitmover.com          http://www.bitmover.com/lm

Re: [PATCH] CodingStyle fixes for drm_agpsupport

[Jeff Garzik]

Larry McVoy wrote:
> On Mon, Aug 11, 2003 at 01:15:58PM -0400, Jeff Garzik wrote:
> 
>>>	if (expr) statement;		// OK
>>
>>The test and the statement run together visually, which is it is 
>>preferred to put the statement on the following line.
> 
> 
> Nah.  
> 
> 	if (!p) return (whatever);
> 	if (foo) {
> 		statement;
> 	} else {
> 		statement;
> 		statement;
> 	}
> 	if (!p) return (whatever);
> 
> Perfectly readable.  We have a few hundred thousand lines of code written

Ug.  The first and last 'if' need spreading out away from the big fat 
block, and the "return (whatever)" fools your eyes into thinking they 
are function calls at a 10-nanosecond glance.  Also, having two styles 
of 'if' formatting in your example just screams "inconsistent" to me :)


> like this and I review all of it.  I suspect that I do more reviewing than
> 99% of the people on this list which makes my opinion count more because
> anything that makes my tired eyes absorb the info faster is a good thing.

Absolutely not.  I'm cooler, so my opinion counts more.


> Same for your eyes when you get to my age.  

I bet when you were in school, you had to chip your homework into slate, 
and dinner was brontosaurus-kebob, right?


> I also make people do
> 
> 	if ((a <= B) || (c >= d)) {
> 		xxx
> 	}
> 
> even though I know, if I think about it, what the precedence is.  It doesn't
> matter that I know or you know, what matters is the number of lines of code
> a day you can correctly review.  Anything that helps that means that you 
> are helping people make the source base better.  Try reading 30K lines of 
> diffs at one sitting and tell me again that I'm wrong.  If you do, bump it
> up to 60K lines :)

Absolutely agreed.  I do the same myself out of habit.


>>>	if (!pointer) return (-EINVAL);
>>>
>>>Short, sweet, readable, no worries.  
>>
>>return is not a function ;-)
> 
> 
> See, there is that age thing again.  Think V6.  And it is sort of a function,
> it unravels the stack frame.

hehe :)  I suppose one could say I'm biased towards the compiler view of 
things, 'return' being a compiler intrinsic, controlling code flow like 
several other compiler intrinsics.

	Jeff, who also despises longjmp()

Re: [PATCH] CodingStyle fixes for drm_agpsupport

[Larry McVoy]

On Mon, Aug 11, 2003 at 01:15:58PM -0400, Jeff Garzik wrote:
> >	if (expr) statement;		// OK
> 
> The test and the statement run together visually, which is it is 
> preferred to put the statement on the following line.

Nah.  

	if (!p) return (whatever);
	if (foo) {
		statement;
	} else {
		statement;
		statement;
	}
	if (!p) return (whatever);

Perfectly readable.  We have a few hundred thousand lines of code written
like this and I review all of it.  I suspect that I do more reviewing than
99% of the people on this list which makes my opinion count more because
anything that makes my tired eyes absorb the info faster is a good thing.
Same for your eyes when you get to my age.  

I also make people do

	if ((a <= B) || (c >= d)) {
		xxx
	}

even though I know, if I think about it, what the precedence is.  It doesn't
matter that I know or you know, what matters is the number of lines of code
a day you can correctly review.  Anything that helps that means that you 
are helping people make the source base better.  Try reading 30K lines of 
diffs at one sitting and tell me again that I'm wrong.  If you do, bump it
up to 60K lines :)

> >	if (!pointer) return (-EINVAL);
> >
> >Short, sweet, readable, no worries.  
> 
> return is not a function ;-)

See, there is that age thing again.  Think V6.  And it is sort of a function,
it unravels the stack frame.
-- 
---
Larry McVoy              lm at bitmover.com          http://www.bitmover.com/lm

Re: [PATCH] CodingStyle fixes for drm_agpsupport

[Jeff Garzik]

Larry McVoy wrote:
> That ought to be balanced with "don't screw up the revision history, people
> use it".  It's one thing to reformat code that is unreadable, for the most
> part this code didn't come close to unreadable.

Granted.


> I wasn't suggesting that.  I was saying
> 
> 	if (expr) statement;		// OK

The test and the statement run together visually, which is it is 
preferred to put the statement on the following line.


> The exception I was saying was reasonable is if you are doing something like
> 
> 	if (!pointer) return (-EINVAL);
> 
> Short, sweet, readable, no worries.  

return is not a function ;-)

	Jeff

Re: [PATCH] CodingStyle fixes for drm_agpsupport

[Larry McVoy]

On Mon, Aug 11, 2003 at 12:58:44PM -0400, Jeff Garzik wrote:
> Larry McVoy wrote:
> >A few comments on why I don't like this patch:
> >    1) It's a formatting only patch.  That screws over people who are using
> >       BK for debugging, now when I double click on these changes I'll get
> >       to your cleanup patch, not the patch that was the last substantive
> >       change.
> 
> This is true, but at the same time, in Linux CodingStyle patches 
> culturally acceptable.  I think the general logic is just "don't go 
> overboard; reformat a tiny fragment at a time."

That ought to be balanced with "don't screw up the revision history, people
use it".  It's one thing to reformat code that is unreadable, for the most
part this code didn't come close to unreadable.

> at least don't run the damn lines together like
> 	if (test) foo else bar;
> 		or
> 	if (test) foo
> 	else bar;

I wasn't suggesting that.  I was saying

	if (expr) statement;		// OK

I was not endorsing this sort of unreadable crap:

	if (expr) statement; else statement;

The exception I was saying was reasonable is if you are doing something like

	if (!pointer) return (-EINVAL);

Short, sweet, readable, no worries.  
-- 
---
Larry McVoy              lm at bitmover.com          http://www.bitmover.com/lm

Re: [PATCH] CodingStyle fixes for drm_agpsupport

[Jeff Garzik]

Larry McVoy wrote:
> A few comments on why I don't like this patch:
>     1) It's a formatting only patch.  That screws over people who are using
>        BK for debugging, now when I double click on these changes I'll get
>        to your cleanup patch, not the patch that was the last substantive
>        change.

This is true, but at the same time, in Linux CodingStyle patches 
culturally acceptable.  I think the general logic is just "don't go 
overboard; reformat a tiny fragment at a time."


>     2) "if (expr) statement;" really ought to be considered legit coding style.
>        It's a one line "shorty" and it lets you see more of the code on a 
>        screen.
>     
> On the other hand, the author carried things too far when they did
> 
> 	if (expr) statement;
> 	else	  statement;
> 
> that's too hard for your eyes to parse quickly IMO.


tee hee :)  This is why we have Documentation/CodingStyle, for just this 
type of discussion.

I actually prefer your "author carried ... too far" example, with the 
reasoning:  if you _must_ deviate from CodingStyle, at least don't run 
the damn lines together like
	if (test) foo else bar;
		or
	if (test) foo
	else bar;

The alignment of the statements visually separates out the test more 
clearly.

	Jeff

Re: [PATCH] CodingStyle fixes for drm_agpsupport

[Larry McVoy]

A few comments on why I don't like this patch:
    1) It's a formatting only patch.  That screws over people who are using
       BK for debugging, now when I double click on these changes I'll get
       to your cleanup patch, not the patch that was the last substantive
       change.
    2) "if (expr) statement;" really ought to be considered legit coding style.
       It's a one line "shorty" and it lets you see more of the code on a 
       screen.
    
On the other hand, the author carried things too far when they did

	if (expr) statement;
	else	  statement;

that's too hard for your eyes to parse quickly IMO.

[PATCH] CodingStyle fixes for drm_agpsupport

[davej]

diff -urpN --exclude-from=/home/davej/.exclude bk-linus/drivers/char/drm/drm_agpsupport.h linux-2.5/drivers/char/drm/drm_agpsupport.h
--- bk-linus/drivers/char/drm/drm_agpsupport.h	2003-07-11 13:57:40.000000000 +0100
+++ linux-2.5/drivers/char/drm/drm_agpsupport.h	2003-07-11 14:08:03.000000000 +0100
@@ -105,7 +105,8 @@ int DRM(agp_acquire)(struct inode *inode
 
 	if (!dev->agp || dev->agp->acquired || !drm_agp->acquire)
 		return -EINVAL;
-	if ((retcode = drm_agp->acquire())) return retcode;
+	if ((retcode = drm_agp->acquire()))
+		return retcode;
 	dev->agp->acquired = 1;
 	return 0;
 }
@@ -142,7 +143,8 @@ int DRM(agp_release)(struct inode *inode
  */
 void DRM(agp_do_release)(void)
 {
-	if (drm_agp->release) drm_agp->release();
+	if (drm_agp->release)
+		drm_agp->release();
 }
 
 /**
@@ -200,7 +202,8 @@ int DRM(agp_alloc)(struct inode *inode, 
 	unsigned long    pages;
 	u32 		 type;
 
-	if (!dev->agp || !dev->agp->acquired) return -EINVAL;
+	if (!dev->agp || !dev->agp->acquired)
+		return -EINVAL;
 	if (copy_from_user(&request, (drm_agp_buffer_t *)arg, sizeof(request)))
 		return -EFAULT;
 	if (!(entry = DRM(alloc)(sizeof(*entry), DRM_MEM_AGPLISTS)))
@@ -222,11 +225,12 @@ int DRM(agp_alloc)(struct inode *inode, 
 	entry->pages     = pages;
 	entry->prev      = NULL;
 	entry->next      = dev->agp->memory;
-	if (dev->agp->memory) dev->agp->memory->prev = entry;
+	if (dev->agp->memory)
+		dev->agp->memory->prev = entry;
 	dev->agp->memory = entry;
 
 	request.handle   = entry->handle;
-        request.physical = memory->physical;
+	request.physical = memory->physical;
 
 	if (copy_to_user((drm_agp_buffer_t *)arg, &request, sizeof(request))) {
 		dev->agp->memory       = entry->next;
@@ -253,7 +257,8 @@ static drm_agp_mem_t *DRM(agp_lookup_ent
 	drm_agp_mem_t *entry;
 
 	for (entry = dev->agp->memory; entry; entry = entry->next) {
-		if (entry->handle == handle) return entry;
+		if (entry->handle == handle)
+			return entry;
 	}
 	return NULL;
 }
@@ -279,12 +284,14 @@ int DRM(agp_unbind)(struct inode *inode,
 	drm_agp_mem_t     *entry;
 	int ret;
 
-	if (!dev->agp || !dev->agp->acquired) return -EINVAL;
+	if (!dev->agp || !dev->agp->acquired)
+		return -EINVAL;
 	if (copy_from_user(&request, (drm_agp_binding_t *)arg, sizeof(request)))
 		return -EFAULT;
 	if (!(entry = DRM(agp_lookup_entry)(dev, request.handle)))
 		return -EINVAL;
-	if (!entry->bound) return -EINVAL;
+	if (!entry->bound)
+		return -EINVAL;
 	ret = DRM(unbind_agp)(entry->memory);
 	if (ret == 0)
 	    entry->bound = 0;
@@ -320,9 +327,11 @@ int DRM(agp_bind)(struct inode *inode, s
 		return -EFAULT;
 	if (!(entry = DRM(agp_lookup_entry)(dev, request.handle)))
 		return -EINVAL;
-	if (entry->bound) return -EINVAL;
+	if (entry->bound)
+		return -EINVAL;
 	page = (request.offset + PAGE_SIZE - 1) / PAGE_SIZE;
-	if ((retcode = DRM(bind_agp)(entry->memory, page))) return retcode;
+	if ((retcode = DRM(bind_agp)(entry->memory, page)))
+		return retcode;
 	entry->bound = dev->agp->base + (page << PAGE_SHIFT);
 	DRM_DEBUG("base = 0x%lx entry->bound = 0x%lx\n",
 		  dev->agp->base, entry->bound);
@@ -351,16 +360,23 @@ int DRM(agp_free)(struct inode *inode, s
 	drm_agp_buffer_t request;
 	drm_agp_mem_t    *entry;
 
-	if (!dev->agp || !dev->agp->acquired) return -EINVAL;
+	if (!dev->agp || !dev->agp->acquired)
+		return -EINVAL;
 	if (copy_from_user(&request, (drm_agp_buffer_t *)arg, sizeof(request)))
 		return -EFAULT;
 	if (!(entry = DRM(agp_lookup_entry)(dev, request.handle)))
 		return -EINVAL;
-	if (entry->bound) DRM(unbind_agp)(entry->memory);
+	if (entry->bound)
+		DRM(unbind_agp)(entry->memory);
+
+	if (entry->prev)
+		entry->prev->next = entry->next;
+	else
+		dev->agp->memory = entry->next;
+
+	if (entry->next)
+		entry->next->prev = entry->prev;
 
-	if (entry->prev) entry->prev->next = entry->next;
-	else             dev->agp->memory  = entry->next;
-	if (entry->next) entry->next->prev = entry->prev;
 	DRM(free_agp)(entry->memory, entry->pages);
 	DRM(free)(entry, sizeof(*entry), DRM_MEM_AGPLISTS);
 	return 0;
@@ -415,14 +431,16 @@ void DRM(agp_uninit)(void)
 /** Calls drm_agp->allocate_memory() */
 struct agp_memory *DRM(agp_allocate_memory)(size_t pages, u32 type)
 {
-	if (!drm_agp->allocate_memory) return NULL;
+	if (!drm_agp->allocate_memory)
+		return NULL;
 	return drm_agp->allocate_memory(pages, type);
 }
 
 /** Calls drm_agp->free_memory() */
 int DRM(agp_free_memory)(struct agp_memory *handle)
 {
-	if (!handle || !drm_agp->free_memory) return 0;
+	if (!handle || !drm_agp->free_memory)
+		return 0;
 	drm_agp->free_memory(handle);
 	return 1;
 }
@@ -430,14 +448,16 @@ int DRM(agp_free_memory)(struct agp_memo
 /** Calls drm_agp->bind_memory() */
 int DRM(agp_bind_memory)(struct agp_memory *handle, off_t start)
 {
-	if (!handle || !drm_agp->bind_memory) return -EINVAL;
+	if (!handle || !drm_agp->bind_memory)
+		return -EINVAL;
 	return drm_agp->bind_memory(handle, start);
 }
 
 /** Calls drm_agp->unbind_memory() */
 int DRM(agp_unbind_memory)(struct agp_memory *handle)
 {
-	if (!handle || !drm_agp->unbind_memory) return -EINVAL;
+	if (!handle || !drm_agp->unbind_memory)
+		return -EINVAL;
 	return drm_agp->unbind_memory(handle);
 }