Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <790eac282623f7a30dac43ff945ce6c0@smtp.hushmail.com>
Date: Sat, 25 Jul 2015 16:41:19 +0200
From: magnum <john.magnum@...hmail.com>
To: john-dev@...ts.openwall.com
Subject: Re: Coding Style

On 2015-07-25 15:05, Kai Zhao wrote:
> There are some rules that some people like while others don't. I am very
> glad to get your advice for each rules whether agree or disagree.Thus, I
> can finish this coding style documentation and there are less coding style
> problems.

Here are my two cents. In general I think we should should have a terse 
and liberal code style, allowing some variations (details below).

> 	Chapter 1: Indentation
>
> 1.1 Tabs are 4 characters for jumbo (8 for core), and thus indentations are
> also 4 characters.

Regardless of what width you use, indentation one (1) tab (per level). 
To me, mentioning a number of characters for indentation is strange and 
incorrectly suggests spaces can be used. I would put it eg. like this:

1.1 Indentation is one tab per level. Recommended tab width is 4 or 8 
for jumbo and 8 for core but it mostly just affects where a line exceeds 
max length.

> 1.2 Indent with tabs, align with spaces. E.g.

OK.

> 1.3 Ease multiple indentation levels in switch(), for(), while()...

OK.

> 1.4 Don't put multiple statements on a single line. A good example is:

OK.

> 1.5 Don't put multiple assignments on a single line either.
>
> Do't do this:
>
> 	i = j = pos = -1;

IMHO we should allow this.

> 	or
>
> 	*pos++ = 0; *ptr = pos;

OK, to me that's repeating 1.4.

> 	Chapter 2: Breaking long lines and strings
>
> The limit on the length of lines is 80 columns.
>
> However, there are some cases that the lines can exceed 80 columns.
> Never break user-visible strings such as printk messages, because
> that breaks the ability to grep for them.

We should mention something other than 'printk'.

> 	Chapter 3: Placing Braces and Spaces
>
> 3.1 Braces
>
> 3.1.1 Function
>
> Put the opening brace at the beginning of the next line, thus:

OK.

> 3.1.2 Others
>
> Put the opening brace last on the next line, thus:
> (...)
>
> Note that the closing brace is empty on a line of its own, _except_ in
> the cases where it is followed by a continuation of the same statement,
> ie a "while" in a do-statement or an "else" in an if-statement, like
> this:
>
> 	do {
> 		body of do-loop
> 	} while (condition);
>
> and
>
> 	if (x == y) {
> 		..
> 	} else if (x > y) {
> 		...
> 	} else {
> 		....
> 	}

IMHO we should allow this too

  	if (x == y) {
  		..
  	}
         else if (x > y) {
  		...
  	}
         else {
  		....
  	}

> 3.1.3 Do not unnecessarily use braces where a single statement will do.

I think we should allow this.

> 3.2 Spaces
>
> 3.2.1 Use a space after (most) keywords.

OK.

> 3.2.2 Do not add spaces around (inside) parenthesized expressions.
>
> This example is *bad*:
>
> 	s = sizeof( struct file );

I never write it like that but maybe we should be more liberal here.

> 3.2.3 When declaring pointer, the preferred use of '*' is adjacent to
> the data name or function name. E.g.:
>
> 	char *linux_banner;
> 	unsigned long long memparse(char *ptr, char **retptr);
> 	char *match_strdup(substring_t *s);

OK.

> 3.2.4 When type conversion, add a space before '*'.
>
> E.g:
>
> 	hostSalt = (cl_uint *) mem_alloc(SALT_BUFFER_SIZE);

I personally disagree. We should mandate NOT adding a space, or just be 
liberal about it. Actually I always drop the space after the paren too:

  	hostSalt = (cl_uint*)mem_alloc(SALT_BUFFER_SIZE);

I think we should allow (not require) a space after the paren, and 
either allow or require that there's space before the '*'.

> 3.2.5 Use one space around (on each side of) most binary and ternary operators,
> such as any of these:

OK.

> 3.2.6 Don't leave whilespace at the end of lines.

OK.

> 	Chapter 4: Naming
>
> 4.1 Names for global variables and global functions
>
> GLOBAL variables (to be used only if you _really_ need them) need to
> have descriptive names, as do global functions.  If you have a function
> that counts the number of active users, you should call that
> "count_active_users()" or similar, you should _not_ call it "cntusr()".

Also: We use names prefixed by crk_ for global functions in cracker.c, 
ldr_ for ones from loader.c and so on. Not sure how we would express 
that though.

> 4.2 Names for local variables
>
> LOCAL variable names should be short, and to the point.  If you have
> some random integer loop counter, it should probably be called "i".
> Calling it "loop_counter" is non-productive, if there is no chance of it
> being mis-understood.  Similarly, "tmp" can be just about any type of
> variable that is used to hold a temporary value.

I think we should drop the above even though I agree with it. We should 
be liberal.

We could mandate against using camelCase but I'm not sure we should do 
that (for Jumbo) either.

> 	Chapter 5: Typedefs
>
> NOTE: This chapter is totally from Kernel CodingStyle. I think many codes
> of john do't follow this rule. So should we follow this rule ?
>
> Please don't use things like "vps_t".
> It's a _mistake_ to use typedef for structures and pointers. When you see a

For our code, I disagree.

> 	Chapter 6: Functions

Our code style should not be a programming tutorial. IMHO drop all of 
chapter 6.

> 	Chapter 7: Centralized exiting of functions

Same here.

> 	Chapter 8: Commenting
>
> 8.1 C89 style and C99 style
>
> John core and Linux style for comments is the C89 /* ... */

I'm not overly happy about mandating this in Jumbo.

> 8.2 Comment content

Our code style should not be a programming tutorial. IMHO drop this.

> 	Chapter 9: Macros, Enums
>
> 9.1 Names of macros defining constants and labels in enums are capitalized.
>
> 	#define CONSTANT 0x12345
>
> Enums are preferred when defining several related constants.
>
> CAPITALIZED macro names are appreciated but macros resembling functions
> may be named in lower case.

OK. I'm not sure we need this although I agree with it.

> Generally, inline functions are preferable to macros resembling functions.

Maybe drop this.

> 9.2 Macros with multiple statements should be enclosed in a do - while block:
>
> 	#define macrofun(a, b, c) 			\
> 		do {					\
> 			if (a == 5)			\
> 				do_this(b, c);		\
> 		} while (0)

This is going towards "tutorial" again and I dislike that.

> 9.3 Things to avoid when using macros:

Same here. These are good advices but I think they have no place in our 
coding style.

> 	Chapter 10: The inline disease
>
> A reasonable rule of thumb is to not put inline at functions that have more
> than 3 lines of code in them. An exception to this rule are the cases where
> a parameter is known to be a compiletime constant, and as a result of this
> constantness you *know* the compiler will be able to optimize most of your
> function away at compile time.

While this is probably reasonable advice, I think we don't need it in 
our code style.

> 	Chapter 11: Function return values and names

I think this does not apply to JtR at all.

magnum

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.