2003-08-11 16:14:07

by Dave Jones

[permalink] [raw]
Subject: [PATCH] CodingStyle fixes for drm_agpsupport

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);
}


2003-08-11 16:41:52

by Larry McVoy

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle fixes for drm_agpsupport

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.

2003-08-11 17:04:23

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle fixes for drm_agpsupport

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


2003-08-11 17:11:32

by Larry McVoy

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle fixes for drm_agpsupport

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

2003-08-11 17:17:57

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle fixes for drm_agpsupport

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



2003-08-11 17:27:52

by Larry McVoy

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle fixes for drm_agpsupport

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

2003-08-11 17:56:02

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle fixes for drm_agpsupport

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()



2003-08-11 18:12:09

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle fixes for drm_agpsupport

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



2003-08-11 18:02:34

by Larry McVoy

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle fixes for drm_agpsupport

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

2003-08-11 19:11:49

by Philip Brown

[permalink] [raw]
Subject: Re: [Dri-devel] Re: [PATCH] CodingStyle fixes for drm_agpsupport

On Mon, Aug 11, 2003 at 10:59:41AM -0700, Larry McVoy wrote:
>...
> It is inconsistent, on purpose. It's essentially like perl's
>
> return unless pointer;
>
> which is a oneliner, almost like an assert().

perl is EEeeeeevil....



> 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?


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


because it's consistent, and guaranteed safe from stupid parsing errors
that can waste days of debug time when someone decides to add to it.
("its just a little change that cant hurt anything", ha ha)


Style Matters. (and so do comments, while we're on the subject)

2003-08-12 09:52:08

by Ed Cogburn

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle fixes for drm_agpsupport

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.

2003-08-12 10:00:49

by Werner Almesberger

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle fixes for drm_agpsupport

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 [email protected] /
/_http://www.almesberger.net/____________________________________________/

2003-08-12 12:07:26

by Firefly

[permalink] [raw]
Subject: Re: [Dri-devel] Re: [PATCH] CodingStyle fixes for drm_agpsupport

On Mon, 11 Aug 2003, Philip Brown wrote:

> if (!pointer) {
> return (whatever);
> }
>
>
> because it's consistent, and guaranteed safe from stupid parsing errors
^^^^^^^^^^^^^^^^^^^^^

return is /still/ not a function - so don't put in visual stuff that hints
that it is.

I can read it and understand it just fine -- if I slow down and think.
Don't make me think unless I absolutely have to. It might distract me
from more important aspects of the code and it steals my time.

-Peter

2003-08-13 19:45:57

by Jamie Lokier

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle fixes for drm_agpsupport

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

2003-08-14 14:21:58

by Eli Carter

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle fixes for drm_agpsupport

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 `-------------------------------------------------

2003-08-14 14:47:39

by Larry McVoy

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle fixes for drm_agpsupport

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

2003-08-14 15:18:50

by Eli Carter

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle fixes for drm_agpsupport

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 `-------------------------------------------------

2003-08-14 15:29:13

by Larry McVoy

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle fixes for drm_agpsupport

> >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

2003-08-14 18:43:51

by Philip Brown

[permalink] [raw]
Subject: Re: [Dri-devel] Re: [PATCH] CodingStyle fixes for drm_agpsupport

On Thu, Aug 14, 2003 at 07:47:11AM -0700, Larry McVoy wrote:
> ...
> 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 :)


You should allow for changes that are "formatting change only", with no
actual code structural change.
You could pass the results through stage 1 of gcc, and only allow it if the
parsing tree is identical.

I was originally going to suggest just passing it through "indent", but
that would not come out right, if someone added braces to clarify a
one-line conditional.

2003-08-14 19:01:47

by Gene Heskett

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle fixes for drm_agpsupport

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.

2003-08-14 19:03:42

by Randy.Dunlap

[permalink] [raw]
Subject: Re: [Dri-devel] Re: [PATCH] CodingStyle fixes for drm_agpsupport

On Thu, 14 Aug 2003 11:43:40 -0700 Philip Brown <[email protected]> wrote:

| On Thu, Aug 14, 2003 at 07:47:11AM -0700, Larry McVoy wrote:
| > ...
| > 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 :)
|
|
| You should allow for changes that are "formatting change only", with no
| actual code structural change.
| You could pass the results through stage 1 of gcc, and only allow it if the
| parsing tree is identical.
|
| I was originally going to suggest just passing it through "indent", but
| that would not come out right, if someone added braces to clarify a
| one-line conditional.

I don't think that BK should know/recognize format-only changes.
However, it would be nice when using bk revtool, one could be looking
at a few targeted lines of a changeset and then click [Prev Rev][Next Rev]
and see the same lines in previous/next revisions of the file.
And if it already does this, great. How do I do that?
I know how to left-click rev-A and right-click rev-B to see changes
between them, but then I have to search thru potentially several 100
or 1000 lines of code for the 10 lines that I'm looking for.
(and it would be nice if one could choose to have the lines numbered).

--
~Randy [who thinks this should be on bk-users]

2003-08-14 20:16:25

by Larry McVoy

[permalink] [raw]
Subject: Re: [Dri-devel] Re: [PATCH] CodingStyle fixes for drm_agpsupport

On Thu, Aug 14, 2003 at 11:43:40AM -0700, Philip Brown wrote:
> On Thu, Aug 14, 2003 at 07:47:11AM -0700, Larry McVoy wrote:
> > ...
> > 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 :)
>
> You should allow for ...

Didn't you mean "in the SCM system I'm writing, I allow for ..."?

Besides, your point is content specific. People check things other than
C code into BK.
--
---
Larry McVoy lm at bitmover.com http://www.bitmover.com/lm