Question about style when converting from K&R to ANSI C.

Greetings all,

I've been converting the few files with old-style function prototypes to ANSI C, and I would like to make sure the following is acceptable.

Original form:

int
foo()
{
/* body here */
}

Proposed conversion:

int foo(void)
{
/* body here */
}

The above should be straightforward, but if there are any problems with that, please holler. I'll be sending patches through the maintainers soon.

Steven

Em Sat, May 31, 2003 at 11:56:16PM -0600, Steven Cole escreveu:

Perfect!

- Arnaldo

Why not just do this then;

Index: linux-2.5/scripts/Lindent
===================================================================
RCS file: /home/cvs/linux-2.5/scripts/Lindent,v
retrieving revision 1.16

diff -u -p -B -r1.16 Lindent
@@ -1,2 +1,2 @@
 #!/bin/sh
-indent -kr -i8 -ts8 -sob -l80 -ss -bs -psl "$@"
+indent -kr -i8 -ts8 -sob -l80 -ss -bs "$@"

function.linuxpower.ca

It shan't be missed ;)

Index: linux-2.5/scripts/Lindent
===================================================================
RCS file: /home/cvs/linux-2.5/scripts/Lindent,v
retrieving revision 1.16

diff -u -p -B -r1.16 Lindent
@@ -1,2 +1,2 @@
 #!/bin/sh
-indent -kr -i8 -ts8 -sob -l80 -ss -bs -psl "$@"
+indent -kr -i8 -ts8 -sob -ss -bs "$@"

function.linuxpower.ca

Sometimes it is nice to be able to see function names with a

	grep '^[a-zA-Z].*(' *.c

which is why I've always preferred

int
foo(void)

{
	/* body here */
}	

Is there some reason that I'm missing that the kernel folks like it the other way?

---
Larry McVoy              lm at bitmover.com          http://www.bitmover.com/lm

Hi Larry !

This will return 'int foo(void)', what's the problem ?

It will only return 'foo(void)', and you won't find its return type. Personally, I strongly prefer getting maximum information in one line, and I find it useful to have the return type, the name and the args together.

If you still need the name and only the name, use some sed on the output :

sed 's/^\([^ ]* \)*\([^]*\)(.*/\2/'

Cheers,
Willy

You get a lot of other false hits, like globals. I don't feel strongly about this, I'm more wondering why this style was choosen. The way I showed is pretty common, it's sort of the "Unix" way (it's how the original Unix guys did it, how BSD did it, and how the GNU guys do it), so it's a somewhat surprising difference. I've never understood the logic. The more I think about it the less I understand it, doing it that way means you are more likely to have to wrap a function definition which is ugly:

static inline int cdrom_write_check_ireason(ide_drive_t *drive, int len, int ireason) {
}

vs

static inline int
cdrom_write_check_ireason(ide_drive_t *drive, int len, int ireason) {
}

It may be just what you are used to but I also find that when reading lots of code it is nice to have it look like

return type
function_name(args)

because the function_name() stands out more, it's always at the left side so I tend to parse it a little more quickly.

Don't get me wrong, I'm not arguing that you should go reformat all your code (I tend to agree with Linus, if it's not your code, don't stick your fingers in there just because you want to reformat it). All I'm doing is trying to understand why in this instance did Linux diverage from common practice.

---
Larry McVoy              lm at bitmover.com          http://www.bitmover.com/lm

Well, I think it's the _main_ reason for most of us coding this way. When you're used to read it this way, it seems completely normal, and other methods seem strange. I even used to put the opening brace on the same line as the function, because I don't like having a line with a single char, I find it a wast of screen space. But the kernel coding style slowly makes me move forward to its method.

I can agree with you on this point. It's only that since I'm not used to read it this way, I have to make an effort finding the type, even if it's just above. I will try to use this method just to see if I can feel comfortable with it.

I just found through google that C programs are indeed formated as you say, but C++ programs have the type on the same line as the name. So if this comes from this origin, we'll be able to say that Linux contains no C++ except its formating :-)

Cheers,
Willy

OK, here is a little more divergence from common practice, but please don't throw too many stones (at least not large ones). The following style was invoked by Linus recently in the first of the zlib conversions.

Here is a snippet of a patch from arch/ppc/xmon/ppc-opc.c:

@@ -420,19 +420,19 @@

    same.  */

/*ARGSUSED*/
-static unsigned long
-insert_bba (insn, value, errmsg)

-     unsigned long insn;
-     long value;
-     const char **errmsg;
+static unsigned long insert_bba(
+       unsigned long insn,
+       long value,
+       const char **errmsg

+)
{
return insn | (((insn >> 16) & 0x1f) << 11);
}

One added bonus of the above style is that it leaves the line count unchanged. ;)

Steven

Of the following, the original is clearly outdated so we can all agree that can go. I'm not real found of Linus' style either. What's wrong with the two traditional forms?

/* ============== original ============== */
static unsigned long
insert_bba (insn, value, errmsg)

	unsigned long insn;
	long value;
	const char **errmsg;
{
	return insn | (((insn >> 16) & 0x1f) << 11);
}

/* ============== linus ============== */
static unsigned long insert_bba(

	unsigned long insn;
	long value;
	const char **errmsg;
)
{
	return insn | (((insn >> 16) & 0x1f) << 11);
}

/* ============== traditional ============== */ static unsigned long
insert_bba(unsigned long insn; long value; const char **errmsg)

{
	return insn | (((insn >> 16) & 0x1f) << 11);
}

/* ============== traditional (lotso args) ============== */ static unsigned long
insert_bba(

	register unsigned const int some_big_fat_variable_name;
	unsigned long insn;
	long value;
	const char **errmsg)
{
	return insn | (((insn >> 16) & 0x1f) << 11);
}

---
Larry McVoy              lm at bitmover.com          http://www.bitmover.com/lm

Umm, I think those ";" should be "," otherwise you get a parameter `insn' has just a forward declaration or some such error.

I have used more traditional style where the new Linus style was not warranted. Here is the patch for fs/jfs/jfs_xtree.c:

@@ -4225,8 +4225,7 @@
  *      at the current entry at the current subtree root page
  *
  */
-int xtGather(t)
-btree_t *t;
+int xtGather(btree_t *t)
 {
 	int rc = 0;
 	xtpage_t *p;

I haven't yet sent that to the maintainer (worked until late last night and still getting -ENOTENOUGHCOFFEE from brain).

Anyway, I agree that more traditional styles should be used unless otherwise indicated, but having the return type on the same line as the function name is something I've warmed up to. And I can remember 14-character filenames and being able to print out the entire kernel in less than 20 minutes on the line printer. That was 8 or 9 years before linux 0.01. Yes, I'm an old-fogey.

Steven

OK, whatever. But are you planning on trying to reformat the kernel and get that pushed into the mainline? That's a fool's errand for lots of reasons. Nobody is going to get excited about having to look through tons of patches which are all white space changes. And it screws up the revision history. Annotated listings and being able to go from that to the patch are a nice thing. If you get all this stuff applied you are hiding the real authorship of each of these function declarations.

---
Larry McVoy              lm at bitmover.com          http://www.bitmover.com/lm

Nope. I'm just doing the absolute minimum. Others have suggested using Lindent and friends, but that would result in the undesirable side-effects you've pointed out, and for little or no gain. So, I for one, will _not_ be doing that.

Obfuscating real authorship is a definite problem however. But I'm confident that you and your Bitkeeper elves can come up with a good solution to this. Surely this problem must slop over into other areas.

Steven

Given that that file is a direct lift from binutils, I would rather update it with the latest version from binutils than waste time on reformatting, if it really bothers you.

My opinion is that changing code that works and that doesn't need attention is a waste of time. If you're working on some code (or even if you are just trying to understand it and you want to make it clearer), then fine, reformat/re-indent/fix argument declarations/whatever, but if you're not, find something more productive to do.

Paul.

The only purpose was to convert from K&R style:

int foo(bar, baz)
long bar;
long baz;
{
}

to ANSI C style:

int foo(long bar, long baz)
{
}

The purpose of the above is that Linus's new sparse checker purposefully ignores K&R style, and he has indicated a willingness to accept patches for the conversion. In fact, he began the conversion himself.

If you'd rather me not touch arch/ppc/xmon/ppc-opc.c and xmon.c, then I won't. I was about ready to send those patches to you. Please let me know what you'd prefer. The default will be no action. Thanks.

Steven

Um... ich! (ignoring the return type debate)

static unsigned long
insert_bba(unsigned long insn,
long value,
const char **errmsg)
{
return insn | (((insn >> 16) & 0x1f) << 11);
}

...OR...

static unsigned long insert_bba(unsigned long insn,
long value, const char **errmsg)
{
return insn | (((insn >> 16) & 0x1f) << 11);
}

NEVER line up the return type and the argument type like the text below. It is mind-clobbering after a couple hundred pages.

Also, though it didn't come up, once you decide to put the arguments on separate lines for readability never mix the styles in one function call.

int X(int A, int B,
char **errormsg)
{
}

is "very bad". the above "looks ok" in that one instance but it isn't. Either I have to count the arguments, or there is one per line, but never seven arguments on five lines (if you please) as that is evil! (It is good for driving instructors crazy too... 8-)

Also, in your automatics, it is never ok to write

int A, *b;
or even
int A, B;

One variable per line please.

int A;
int *b;

(Especially if you are programming, or ever hope to program, tiny little
embedded systems where you really have to visualize your stack requirements. 8-) The real reason is to facilitate and encourage automatic initializations. Particularly of classes in C++, but profoundly so in basic C none the less. Uninitialized variables should look bare and lonely, and perhaps even a tad wrong. (Not to mention having more than one alphabetic character as a name, but I was being minimalist... 8-)

Rob.

-----Original Message-----
From: [email protected] [mailto:[email protected]]On Behalf Of Steven Cole

/*ARGSUSED*/
-static unsigned long
-insert_bba (insn, value, errmsg)

-     unsigned long insn;
-     long value;
-     const char **errmsg;
+static unsigned long insert_bba(
+       unsigned long insn,
+       long value,
+       const char **errmsg

+)
{
return insn | (((insn >> 16) & 0x1f) << 11);
}

The reason I've liked this format is that it gives me a quick and universal way to find *specific* functions with vi or grep, by searching for "^function_name(".

I'm less concerned with global function searches; there I don't mind the overhead of more sophisticated tools.

When we're done with this thread, perhaps we can return to endian arguments....

/Jonathan Lundell.

Exactly. I thought of making that point in my original posting and figured everyone would tell me to use tags and I didn't want to have to remember all the other reasons I wanted this.

It really is nice knowing that "^function_name(" is the definition.

---
Larry McVoy              lm at bitmover.com          http://www.bitmover.com/lm

Thanks for the input. You've convinced me. When going through arch/ppc/xmon/xmon.c, I will leave things like the following unchanged:

/* Command interpreting routine */
static int
cmds(struct pt_regs *excp)
{

My changes will be similar to the following:

@@ -1837,9 +1818,7 @@

        return *lineptr++;

}

-void
-take_input(str)
-char *str;
+void take_input(char *str)

 {
        lineptr = str;

}

I'll be changing the return type/function name line orientation only when making other changes. And these will still go through the applicable maintainers.

Steven

Great.

{
       lineptr = str;
}

---
Larry McVoy              lm at bitmover.com          http://www.bitmover.com/lm

Yeah, I realized the inconsistency of that after sending. This is with that change backed out (same as your example above):

@@ -1838,8 +1819,7 @@
}

void
-take_input(str)
-char *str;
+take_input(char *str)

 {
        lineptr = str;

}

My guiding principle in all this is "first, do no harm". That's why I posted my question in the first place. Thanks for the help.

Steven

Actually, that would most likely be:
static inline int cdrom_write_check_ireason(
ide_drive_t *drive,
int len,
int ireason
)
{
...
}

At least If I were doing it. Over my 20 years, I've found that many of MY type errors are due to returning or expecting the wrong structure/variable because I forgot the type of the function.

I rarely have to look at the parameters (though when I do, I locate them via the function name, then scan the parameters...) sometimes just to count the number of parameters, or the order, which is easier when the parameters are one to a line. Either as in K&R, or the new style.

My personal preference(s) are:

In C or naked scope of C++:

static inline
int function_name(type arg, type arg)
{
body
}

By putting the scope modifiers and compiler directives on one line and the calling conventions on another it makes a distinction between what the user needs to know as opposed to the compiler.

In C++ classes:

class   				ClassName {
	static inline int		function_name(type arg, type arg) { return expression; }
	static inline int       function_name(type arg, type arg) {
						complex body;
					}
					ClassName();
	virtual		     ~ClassName();
};

(If the above doesn't look right because of email handling cruft) The names of the class and members all line up vertically, the destructor is proceeded by seven spaces and a tilde instead of a tab (so the destructor "stands out" in a quick code scan) and the "static" and "inline" in this usage are actually part of the type of the member function of a class in a way that they are only hints in a naked scope, so they move down onto the line itself.

There is no good "find it" trick if you don't do the below, but if you do use the rest of "my personal standard" then doing "egrep ')$'" gets you all of the function definitions with only the occasional split-line-conditional, and since I also "&&" and "||" those at the end of the line I never have that problem either...

(And not that anybody asked)

if (test) {
code
} else {
more code
}

while (test) {
code
}

do {
code
} while (test);

and so on...

and also (for the advanced reader):

if ((conditional) &&

    (conditional)) {
}

This one being "advanced" because it reads like a book but you have to "hold in your head" were you are in the conditional more aggressively than the prefixing version.

These preferences go back to when vi would delete lines if you scrolled down through a file quickly using the arrow keys. Even in the absence of that particular annoyance (because we don't need a vi versus emacs holy war flare up, they are both evil anyway, even if I do use vi constantly 8-), I have never liked the floating brace styles like

  if (test)
  {
     code
  }

because the code still works and even looks right if you accidentally damage/remove the if. Just like "while (test);" on a line by itself is transformed into a hang if the "do" ten pages up is removed.

  if ((conditional)
    &&(conditional))
  {
    code
  }

is subject to problems on line delete too. Accidentally remove the "&&" line and it is really easy to decide you made a parenthesis counting mistake. If the conditional is on the leading line then when you see

  if ((conditional) &&
  {
    code
  }

you instantly know something is amiss.

Fully correct, the dropped line would leave
if ((conditional) &&
code
}

because the leading brace would have been eaten with the bad line too. Neither positioning will protect you from dropping a middle term of three, for obvious reasons.

AND MOST IMPORTANTLY:

while (test)
one line of code;

AND

if (test)
one line of code;

(etc) are fundamentally _*EVIL*_ (evil I say! do you hear me? EVIL!!!!! 8-)

Whatever the language designers might say, braces are not optional in "morally correct" C or C++.

(I have maintained too much crappy code in my life, most of it written by students, to think the other "standards" are anything but accidents waiting to happen. The only reason the floating-brace standard is taught in schools is because it makes red-pen grading easier because of the blank page space.)

Rob.

-----Original Message-----
From: [email protected] [mailto:[email protected]]On Behalf Of Larry McVoy

static inline int cdrom_write_check_ireason(ide_drive_t *drive, int len, int ireason)
{
}

vs

static inline int
cdrom_write_check_ireason(ide_drive_t *drive, int len, int ireason) {
}

Just my personal opinion:

The return value is part of the function signature; placing it on a separate line implies a disconnect between the return value and the rest of the declaration.

It's a matter of psychology; your mileage may vary.

Scott Robert Ladd
Coyote Gulch Productions (http://www.coyotegulch.com)

In article <[email protected]>,

That makes no sense.

Do you write your normal variable definitions like

	int
	a,b,c;

too? No you don't, because that would be totally idiotic.

A function declaration is no different. The type of the function is very important to the function itself (along with the arguments), and I personally want to see _all_ of it when I grep for functions.

You should just do

	grep -i '^[a-z_ ]*(' *.c 

and you'll get a nice function declaration with the standard kernel coding style.

And I personally don't normally do "grep for random function declarations", that just sounds like a contrieved example. I grep for specific function names to find usage, and then it's _doubly_ important to see that the return (and argument) types match and make sense.

So I definitely prefer all the arguments on the same line too, even if that makes the line be closer to 100 chars than 80. The zlib K&R->ANSI conversion was a special case, and I'd be happy if somebody were to have the energy to convert it all the way (which implies moving comments around etc).

Linus

Whatever, it's your tree. I'd rather have you force everything to the same style, any style, than tolerate all sorts of different styles.

---
Larry McVoy              lm at bitmover.com          http://www.bitmover.com/lm

Agree here. Personally I don't care of using whatever non-brain damaged style but I like to see projects using a consistent coding style. Whatever it is ...

- Davide

How is this? I don't know about the energy part as xor'ed with itself leaves the value unchanged right now.

I've tried to follow Documentation/kernel-doc-nano-HOWTO.txt as suggested by acme.

Putting all the arguments on the same line gives 103 characters, if I counted correctly. Others will be longer, so this is chosen as a folded example.

Steven

@@ -288,14 +288,17 @@
   return y != 0 && g != 1 ? Z_BUF_ERROR : Z_OK;
 }
 
+/**
+ *	zlib_inflate_trees_bits:
+ *	@uIntf *c:                19 code lengths
+ *	@uIntf *bb:               bits tree desired/actual depth
+ *	@inflate_huft * FAR *tb:  bits tree result
+ *	@inflate_huft *hp:        space for trees
+ *	@z_streamp z:             for messages
+ */
 
-int zlib_inflate_trees_bits(
-	uIntf *c,               /* 19 code lengths */
-	uIntf *bb,              /* bits tree desired/actual depth */
-	inflate_huft * FAR *tb, /* bits tree result */
-	inflate_huft *hp,       /* space for trees */
-	z_streamp z             /* for messages */
-)
+int zlib_inflate_trees_bits(uIntf *c, uIntf *bb, inflate_huft * FAR *tb,
+	inflate_huft *hp, z_streamp z)
 {
   int r;
   uInt hn = 0;          /* hufts used in space */

But it does. Type /^foo <enter> and you're at the function definition. At least when using vi, which is the editor everybody's using, right? ;-)

Also, when in working in a (too) long function body, type ?^{ and you're at the start of the function body.

Indeed, searching for ^a will fail. There's no reason whatsoever why you'd declare your variables that way.

Erik Hensema <[email protected]>

hi :)

well, but it is nice to be able to grep for the declaration of a function like

	grep "^where_is_it" *.c

without showing all the uses of that function.

also, putting the return type on its own line makes sure that the function name is always at the same position. this greatly enhances readability (at least for me ;)

CU,		  / Friedrich-Alexander University Erlangen, Germany
Martin Waitz	//  Department of Computer Science 3       _________
______________/// - - - - - - - - - - - - - - - - - - - - ///
dies ist eine manuell generierte mail, sie beinhaltet    //
tippfehler und ist auch ohne grossbuchstaben gueltig.   /

What's wrong with ctags for this?

Dave

- You have to set it up (hackers are lazy)
- You generate an extra file that some people might not want

imo it doesn't really matter much either way. Whatever you prefer, there are greater evils in the kernel, even in relatively important parts. So why bother with such minor and highly personal issues.

Jörn

Jörn Engel
mailto: [email protected]
http://wohnheim.fh-wedel.de/~joern
Phone: +49 179 6704074

<sarcasm>
Bah, all this newfangled crap like ctags. We've used grep for 30 years and there is no reason to change this now.
</sarcasm>

Dave, the arguments some people bring to simply cling to their formatting reminds me of the arguments that the church had in the 14th century to still prove that the sun revolves around the earth. Simply ignore them. I'm grateful that there are programming environments beyond vi. [1] :-)

Regards
Henning

[1] emacs

Dipl.-Inf. (Univ.) Henning P. Schmiedehausen          INTERMETA GmbH
[email protected]        +49 9131 50 654 0   http://www.intermeta.de/

Java, perl, Solaris, Linux, xSP Consulting, Web Services 
freelance consultant -- Jakarta Turbine Development  -- hero for hire

[SNIPPED...]

(1) ed

Spraying garbage all over source code destroys editor agnosticism, even if some editor exists that can hide all the crap like trailing whitespace, spaces where tabs belong, and screwed-up indentation. Use emacs all you want. Don't force others to use some particular editor by spewing garbage all over the kernel source that requires some special editor to hide.

-- wli

The footnote was a non-native language speakers' attempt at self-irony.

If normal people think about development environments beyond vi, they would consider VisualStudio an improvement. Or eclipse.

Trust me, "whitespace" or "tabs" are the smallest of your problems there.

Ah well, silly me, trying humour on LKLM. ;-)

Regards
Henning

Dipl.-Inf. (Univ.) Henning P. Schmiedehausen          INTERMETA GmbH
[email protected]        +49 9131 50 654 0   http://www.intermeta.de/

Java, perl, Solaris, Linux, xSP Consulting, Web Services 
freelance consultant -- Jakarta Turbine Development  -- hero for hire

We're not normal people. We're UNIX kernel hackers. We often do things in crusty old ways.

Dead serious. Mandating emacs and/or some UNIX-based Visual C++ workalike in order to cope with utter garbage code formatting is not going to fly.

-- wli

You did good (or even well considering that you are a non-native speaker). Yes, whitespace is a trivial problem.

~Randy

On 03 Jun 2003 16:44:05 +0200 Henning Schmiedehausen <[email protected]> wrote:

| The footnote was a non-native language speakers' attempt at self-irony.
| 
| If normal people think about development environments beyond vi, they
| would consider VisualStudio an improvement. Or eclipse.
| 
| Trust me, "whitespace" or "tabs" are the smallest of your problems
| there.
| 
| Ah well, silly me, trying humour on LKLM. ;-)
| 
| 	Regards
| 		Henning
| 
| 
| On Tue, 2003-06-03 at 15:39, William Lee Irwin III wrote:
| > On Tue, Jun 03, 2003 at 01:18:43PM +0000, Henning P. Schmiedehausen wrote:
| > > <sarcasm>
| > > Bah, all this newfangled crap like ctags. We've used grep for 30 years
| > > and there is no reason to change this now. 
| > > </sarcasm>
| > > Dave, the arguments some people bring to simply cling to their
| > > formatting reminds me of the arguments that the church had in the 14th
| > > century to still prove that the sun revolves around the earth. Simply
| > > ignore them. I'm grateful that there are programming environments
| > > beyond vi. [1] :-)
| > > 	Regards
| > > 		Henning
| > > [1] emacs 
| > 
| > Spraying garbage all over source code destroys editor agnosticism, even
| > if some editor exists that can hide all the crap like trailing
| > whitespace, spaces where tabs belong, and screwed-up indentation. Use
| > emacs all you want. Don't force others to use some particular editor by
| > spewing garbage all over the kernel source that requires some special
| > editor to hide.
| > 
| > 
| > -- wli

Bad whitespace, along with all other coding style violations, is a very serious maintainability problem.

-- wli

On Tue, 3 Jun 2003 08:38:59 -0700 William Lee Irwin III <[email protected]> wrote:

| On Tue, Jun 03, 2003 at 08:25:05AM -0700, Randy.Dunlap wrote:
| > You did good (or even well considering that you are a
| > non-native speaker).  Yes, whitespace is a trivial problem.
| > ~Randy
| 
| Bad whitespace, along with all other coding style violations, is
| a very serious maintainability problem.

I agree. I mean that it's a trivial problem to fix... if the patches were to be accepted. But that's another problem.

--
~Randy