Hi,
out of curiosity I tried running indent on the whole source tree, and then running abuild to see whether things break. Short answer: they do.
I used thisîlittle script to indent the code:
for f in `find . -name '*.[ch]'`; do indent -kr -i8 -ts8 -sob -l80 -ss -ncs $f done
abuild fails because some code will be broken in the process, and some code will still work but look ugly.
There are workarounds though, you can enclose paragraphs which should not be touched by indent as follows:
/* *INDENT-OFF* */
/* *INDENT-ON* */
I suggest we should agree on some set of parameters for indent, add INDENT-OFF/INDENT-ON to the code where appropriate and then, as soon as abuild doesn't complain anymore, checkin the indented code.
New checkins which don't conform to the coding style (and other guidelines which may apply) should not be allowed from that point on.
Comments?
Uwe.
Uwe Hermann wrote:
New checkins which don't conform to the coding style (and other guidelines which may apply) should not be allowed from that point on.
Comments?
I like it but what on earth is indent breaking? I don't like the idea of special comments. What's going on?
thanks!
ron
* Ronald G Minnich rminnich@lanl.gov [061006 20:29]:
Uwe Hermann wrote:
New checkins which don't conform to the coding style (and other guidelines which may apply) should not be allowed from that point on.
Comments?
I like it but what on earth is indent breaking? I don't like the idea of special comments. What's going on?
Can we call this manually and fix the few things up before checkin?
Also I am not sure if we really want to break at 80 columns, even though we really should ;-)
Hi,
On Fri, Oct 06, 2006 at 08:56:30PM +0200, Stefan Reinauer wrote:
Can we call this manually and fix the few things up before checkin?
I guess so, but I'd rather temporarily use the INDENT-OFF markers where needed and then remove them. That's less work than manually fixing all the stuff indent changed which it shouldn't have changed.
Also I am not sure if we really want to break at 80 columns, even though we really should ;-)
I'd say we should, at least in most cases. Sometimes that may be not so good; in such cases leave the code as is (IMHO).
Uwe.
Hi,
On Fri, Oct 06, 2006 at 12:29:46PM -0600, Ronald G Minnich wrote:
I like it but what on earth is indent breaking? I don't like the idea of special comments. What's going on?
One example I noticed:
- dev = pci_locate_device_on_bus( - PCI_ID(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_8111_ISA), - bus); + dev = + pci_locate_device_on_bus(PCI_ID + (PCI_VENDOR_ID_AMD, + PCI_DEVICE_ID_AMD_8111_ISA), bus);
As PCI_ID is a macro breaking it overîmultiple lines seems to cause trouble (in this case at least).
Uwe.
On Sat, Oct 07, 2006 at 11:31:11PM +0200, Uwe Hermann wrote:
One example I noticed:
dev = pci_locate_device_on_bus(PCI_ID(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_8111_ISA),bus);
dev =pci_locate_device_on_bus(PCI_ID(PCI_VENDOR_ID_AMD,PCI_DEVICE_ID_AMD_8111_ISA), bus);As PCI_ID is a macro breaking it over multiple lines seems to cause trouble (in this case at least).
Macro or no, I don't like the above change at all. I think the former is much more readable.
//Peter
* Peter Stuge stuge-linuxbios@cdy.org [061008 00:58]:
As PCI_ID is a macro breaking it over multiple lines seems to cause trouble (in this case at least).
Macro or no, I don't like the above change at all. I think the former is much more readable.
this is one of the nasty places indeed. but very often indent causes code to be much more readable. Just not sure if we can just run indent and check things on or if this requires a persevering hand job.
Macro or no, I don't like the above change at all. I think the former is much more readable.
Yah.
this is one of the nasty places indeed. but very often indent causes code to be much more readable. Just not sure if we can just run indent and check things on or if this requires a persevering hand job.
If you want to run indent at all, do it _once_ (with all the right options! See lindent) and fix up the fallout later, whenever you stumble on it -- that's the least-effort way to go about it.
Segher
* Segher Boessenkool segher@kernel.crashing.org [061008 01:52]:
If you want to run indent at all, do it _once_ (with all the right options! See lindent) and fix up the fallout later, whenever you stumble on it -- that's the least-effort way to go about it.
some of our specialists send in patches that consist of 50% whitespace changes because they are using broken editors that convert tab to space.
question is how to cope with that, especially if those people have commit rights themselfes and dont wait for a review to happen.
If you want to run indent at all, do it _once_ (with all the right options! See lindent) and fix up the fallout later, whenever you stumble on it -- that's the least-effort way to go about it.
some of our specialists send in patches that consist of 50% whitespace changes because they are using broken editors that convert tab to space.
question is how to cope with that, especially if those people have commit rights themselfes and dont wait for a review to happen.
pre-commit hook, run a regexp that finds the more standard problems, refuse if bad.
The Linux kernel source code has some such regexp's for you.
But do you really want to completely lock out "bad-looking" whitespace changes?
You can't solve social problems using technology; you'll have to change people's habits instead. *Every* patch should be sent to the mailing list for review, it doesn't matter who you are.
Segher
Hi,
On Sun, Oct 08, 2006 at 04:24:10PM +0200, Segher Boessenkool wrote:
You can't solve social problems using technology; you'll have to change people's habits instead. *Every* patch should be sent to the mailing list for review, it doesn't matter who you are.
Yep, definately.
We should write a wiki page containing some development guidelines ASAP and then point people to it and encourage everyone to follow them.
Uwe.
-
Peter Stuge -
Ronald G Minnich -
Segher Boessenkool -
Stefan Reinauer -
Uwe Hermann