* RE: [Dri-devel] Re: [PATCH] CodingStyle fixes for drm_agpsupport
@ 2003-08-11 20:21 Sottek, Matthew J
2003-08-12 7:11 ` Ryan Anderson
2003-08-12 12:05 ` Peter "Firefly" Lund
0 siblings, 2 replies; 10+ messages in thread
From: Sottek, Matthew J @ 2003-08-11 20:21 UTC (permalink / raw)
To: dri-devel; +Cc: linux-kernel
> 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)
I've always been an "Always use brackets" evangelist for two
reasons.
1) The time it takes to write the code fragment is noise
compared to the amount of cumulative time that everyone else
will spend reading it over it's lifespan. This is more true
in open source than it ever was in the closed source world.
Making the code explicit saves everyone time in the longrun.
2) There are some very real ways that bracketless code will
get broken. Either someone adds a line that didn't notice the
lack of brackets or, someone accidentally uses a multi-line
macro.
i.e.
if(foo)
DEBUG_PRINT("Foo!\n");
works great for 100 years until someone recodes the DEBUG_PRINT
macro to be 2 lines. The Linux kernel often has plain looking
functions or variables that end up being macros (and may only
expand to multi-line on some platforms) which could easily get
you into such a situation.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Dri-devel] Re: [PATCH] CodingStyle fixes for drm_agpsupport
2003-08-11 20:21 [Dri-devel] Re: [PATCH] CodingStyle fixes for drm_agpsupport Sottek, Matthew J
@ 2003-08-12 7:11 ` Ryan Anderson
2003-08-12 12:05 ` Peter "Firefly" Lund
1 sibling, 0 replies; 10+ messages in thread
From: Ryan Anderson @ 2003-08-12 7:11 UTC (permalink / raw)
To: linux-kernel
On Mon, Aug 11, 2003 at 01:21:22PM -0700, Sottek, Matthew J wrote:
> 2) There are some very real ways that bracketless code will
> get broken. Either someone adds a line that didn't notice the
> lack of brackets or, someone accidentally uses a multi-line
> macro.
>
> i.e.
> if(foo)
> DEBUG_PRINT("Foo!\n");
>
> works great for 100 years until someone recodes the DEBUG_PRINT
> macro to be 2 lines. The Linux kernel often has plain looking
> functions or variables that end up being macros (and may only
> expand to multi-line on some platforms) which could easily get
> you into such a situation.
#define DEBUG_PRINT(x) do { printk((x)); printk((x)); } while (0)
I believe your example is, oh, the #1 reason for this style of macro.
--
Ryan Anderson
sometimes Pug Majere
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [Dri-devel] Re: [PATCH] CodingStyle fixes for drm_agpsupport
2003-08-11 20:21 [Dri-devel] Re: [PATCH] CodingStyle fixes for drm_agpsupport Sottek, Matthew J
2003-08-12 7:11 ` Ryan Anderson
@ 2003-08-12 12:05 ` Peter "Firefly" Lund
1 sibling, 0 replies; 10+ messages in thread
From: Peter "Firefly" Lund @ 2003-08-12 12:05 UTC (permalink / raw)
To: Sottek, Matthew J; +Cc: dri-devel, linux-kernel
On Mon, 11 Aug 2003, Sottek, Matthew J wrote:
> if(foo)
> DEBUG_PRINT("Foo!\n");
>
> works great for 100 years until someone recodes the DEBUG_PRINT
> macro to be 2 lines.
Then those "someone" shouldn't be allowed near a macro system.
This is the standard trick:
#define DEBUG_PRINT(x) do { \
stmt1; \
stmt2; \
} while (0)
-Peter
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] CodingStyle fixes for drm_agpsupport
@ 2003-08-11 15:59 davej
2003-08-11 16:40 ` Larry McVoy
0 siblings, 1 reply; 10+ messages in thread
From: davej @ 2003-08-11 15:59 UTC (permalink / raw)
To: torvalds; +Cc: linux-kernel, dri-devel
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);
}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] CodingStyle fixes for drm_agpsupport
2003-08-11 15:59 davej
@ 2003-08-11 16:40 ` Larry McVoy
2003-08-11 16:58 ` Jeff Garzik
0 siblings, 1 reply; 10+ messages in thread
From: Larry McVoy @ 2003-08-11 16:40 UTC (permalink / raw)
To: davej; +Cc: torvalds, linux-kernel, dri-devel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] CodingStyle fixes for drm_agpsupport
2003-08-11 16:40 ` Larry McVoy
@ 2003-08-11 16:58 ` Jeff Garzik
2003-08-11 17:04 ` Larry McVoy
0 siblings, 1 reply; 10+ messages in thread
From: Jeff Garzik @ 2003-08-11 16:58 UTC (permalink / raw)
To: Larry McVoy; +Cc: davej, torvalds, linux-kernel, dri-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] CodingStyle fixes for drm_agpsupport
2003-08-11 16:58 ` Jeff Garzik
@ 2003-08-11 17:04 ` Larry McVoy
2003-08-11 17:15 ` Jeff Garzik
2003-08-14 14:21 ` Eli Carter
0 siblings, 2 replies; 10+ messages in thread
From: Larry McVoy @ 2003-08-11 17:04 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Larry McVoy, davej, torvalds, linux-kernel, dri-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] CodingStyle fixes for drm_agpsupport
2003-08-11 17:04 ` Larry McVoy
@ 2003-08-11 17:15 ` Jeff Garzik
2003-08-11 17:23 ` Larry McVoy
2003-08-14 14:21 ` Eli Carter
1 sibling, 1 reply; 10+ messages in thread
From: Jeff Garzik @ 2003-08-11 17:15 UTC (permalink / raw)
To: Larry McVoy; +Cc: davej, torvalds, linux-kernel, dri-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] CodingStyle fixes for drm_agpsupport
2003-08-11 17:15 ` Jeff Garzik
@ 2003-08-11 17:23 ` Larry McVoy
2003-08-11 17:53 ` Jeff Garzik
0 siblings, 1 reply; 10+ messages in thread
From: Larry McVoy @ 2003-08-11 17:23 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Larry McVoy, davej, torvalds, linux-kernel, dri-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] CodingStyle fixes for drm_agpsupport
2003-08-11 17:23 ` Larry McVoy
@ 2003-08-11 17:53 ` Jeff Garzik
2003-08-11 17:59 ` Larry McVoy
0 siblings, 1 reply; 10+ messages in thread
From: Jeff Garzik @ 2003-08-11 17:53 UTC (permalink / raw)
To: Larry McVoy; +Cc: davej, torvalds, linux-kernel, dri-devel
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()
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] CodingStyle fixes for drm_agpsupport
2003-08-11 17:53 ` Jeff Garzik
@ 2003-08-11 17:59 ` Larry McVoy
2003-08-11 19:09 ` [Dri-devel] " Philip Brown
0 siblings, 1 reply; 10+ messages in thread
From: Larry McVoy @ 2003-08-11 17:59 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Larry McVoy, davej, torvalds, linux-kernel, dri-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Dri-devel] Re: [PATCH] CodingStyle fixes for drm_agpsupport
2003-08-11 17:59 ` Larry McVoy
@ 2003-08-11 19:09 ` Philip Brown
2003-08-12 12:07 ` Peter "Firefly" Lund
0 siblings, 1 reply; 10+ messages in thread
From: Philip Brown @ 2003-08-11 19:09 UTC (permalink / raw)
To: dri-devel; +Cc: linux-kernel
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)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Dri-devel] Re: [PATCH] CodingStyle fixes for drm_agpsupport
2003-08-11 19:09 ` [Dri-devel] " Philip Brown
@ 2003-08-12 12:07 ` Peter "Firefly" Lund
0 siblings, 0 replies; 10+ messages in thread
From: Peter "Firefly" Lund @ 2003-08-12 12:07 UTC (permalink / raw)
To: Philip Brown; +Cc: dri-devel, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] CodingStyle fixes for drm_agpsupport
2003-08-11 17:04 ` Larry McVoy
2003-08-11 17:15 ` Jeff Garzik
@ 2003-08-14 14:21 ` Eli Carter
2003-08-14 14:47 ` Larry McVoy
1 sibling, 1 reply; 10+ messages in thread
From: Eli Carter @ 2003-08-14 14:21 UTC (permalink / raw)
To: Larry McVoy; +Cc: Jeff Garzik, davej, torvalds, linux-kernel, dri-devel
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 `-------------------------------------------------
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] CodingStyle fixes for drm_agpsupport
2003-08-14 14:21 ` Eli Carter
@ 2003-08-14 14:47 ` Larry McVoy
2003-08-14 18:43 ` [Dri-devel] " Philip Brown
0 siblings, 1 reply; 10+ messages in thread
From: Larry McVoy @ 2003-08-14 14:47 UTC (permalink / raw)
To: Eli Carter
Cc: Larry McVoy, Jeff Garzik, davej, torvalds, linux-kernel, dri-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Dri-devel] Re: [PATCH] CodingStyle fixes for drm_agpsupport
2003-08-14 14:47 ` Larry McVoy
@ 2003-08-14 18:43 ` Philip Brown
2003-08-14 18:59 ` Randy.Dunlap
2003-08-14 20:16 ` Larry McVoy
0 siblings, 2 replies; 10+ messages in thread
From: Philip Brown @ 2003-08-14 18:43 UTC (permalink / raw)
To: dri-devel
Cc: Larry McVoy, Eli Carter, Larry McVoy, Jeff Garzik, davej,
torvalds, linux-kernel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Dri-devel] Re: [PATCH] CodingStyle fixes for drm_agpsupport
2003-08-14 18:43 ` [Dri-devel] " Philip Brown
@ 2003-08-14 18:59 ` Randy.Dunlap
2003-08-14 20:16 ` Larry McVoy
1 sibling, 0 replies; 10+ messages in thread
From: Randy.Dunlap @ 2003-08-14 18:59 UTC (permalink / raw)
To: Philip Brown
Cc: dri-devel, lm, eli.carter, lm, jgarzik, davej, torvalds, linux-kernel
On Thu, 14 Aug 2003 11:43:40 -0700 Philip Brown <phil@bolthole.com> 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]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Dri-devel] Re: [PATCH] CodingStyle fixes for drm_agpsupport
2003-08-14 18:43 ` [Dri-devel] " Philip Brown
2003-08-14 18:59 ` Randy.Dunlap
@ 2003-08-14 20:16 ` Larry McVoy
2003-08-14 20:21 ` Eli Carter
1 sibling, 1 reply; 10+ messages in thread
From: Larry McVoy @ 2003-08-14 20:16 UTC (permalink / raw)
To: dri-devel, Eli Carter, Larry McVoy, Jeff Garzik, davej, torvalds,
linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Dri-devel] Re: [PATCH] CodingStyle fixes for drm_agpsupport
2003-08-14 20:16 ` Larry McVoy
@ 2003-08-14 20:21 ` Eli Carter
2003-08-14 20:22 ` Larry McVoy
0 siblings, 1 reply; 10+ messages in thread
From: Eli Carter @ 2003-08-14 20:21 UTC (permalink / raw)
To: Larry McVoy; +Cc: dri-devel, Jeff Garzik, davej, torvalds, linux-kernel
Larry McVoy wrote:
> 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.
I assume you can have content-specific validators run before a commit?
(CVS can.) A validator could see that it was formatting only and mark it
in someway perhaps?
But we've wandered _way_ OT at this point, and it's well past the point
of diminishing returns...
Eli
--------------------. "If it ain't broke now,
Eli Carter \ it will be soon." -- crypto-gram
eli.carter(a)inet.com `-------------------------------------------------
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Dri-devel] Re: [PATCH] CodingStyle fixes for drm_agpsupport
2003-08-14 20:21 ` Eli Carter
@ 2003-08-14 20:22 ` Larry McVoy
0 siblings, 0 replies; 10+ messages in thread
From: Larry McVoy @ 2003-08-14 20:22 UTC (permalink / raw)
To: Eli Carter
Cc: Larry McVoy, dri-devel, Jeff Garzik, davej, torvalds, linux-kernel
On Thu, Aug 14, 2003 at 03:21:04PM -0500, Eli Carter wrote:
> >Besides, your point is content specific. People check things other than
> >C code into BK.
>
> I assume you can have content-specific validators run before a commit?
bk help triggers
--
---
Larry McVoy lm at bitmover.com http://www.bitmover.com/lm
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2003-08-14 20:24 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2003-08-11 20:21 [Dri-devel] Re: [PATCH] CodingStyle fixes for drm_agpsupport Sottek, Matthew J
2003-08-12 7:11 ` Ryan Anderson
2003-08-12 12:05 ` Peter "Firefly" Lund
-- strict thread matches above, loose matches on Subject: below --
2003-08-11 15:59 davej
2003-08-11 16:40 ` Larry McVoy
2003-08-11 16:58 ` Jeff Garzik
2003-08-11 17:04 ` Larry McVoy
2003-08-11 17:15 ` Jeff Garzik
2003-08-11 17:23 ` Larry McVoy
2003-08-11 17:53 ` Jeff Garzik
2003-08-11 17:59 ` Larry McVoy
2003-08-11 19:09 ` [Dri-devel] " Philip Brown
2003-08-12 12:07 ` Peter "Firefly" Lund
2003-08-14 14:21 ` Eli Carter
2003-08-14 14:47 ` Larry McVoy
2003-08-14 18:43 ` [Dri-devel] " Philip Brown
2003-08-14 18:59 ` Randy.Dunlap
2003-08-14 20:16 ` Larry McVoy
2003-08-14 20:21 ` Eli Carter
2003-08-14 20:22 ` Larry McVoy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).