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