RE: [Dri-devel] Re: [PATCH] CodingStyle fixes for drm_agpsupport

RE: [Dri-devel] Re: [PATCH] CodingStyle fixes for drm_agpsupport

[Sottek, Matthew J]

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

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

[Ryan Anderson]

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

RE: [Dri-devel] Re: [PATCH] CodingStyle fixes for drm_agpsupport

[Peter "Firefly" Lund]

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

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

[Larry McVoy]

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

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

[Eli Carter]

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

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

[Larry McVoy]

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

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

[Randy.Dunlap]

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]

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

[Philip Brown]

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.

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

[Peter "Firefly" Lund]

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

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

[Philip Brown]

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)