|
Packit Service |
31306d |
Coding conventions in the libssh tree
|
|
Packit Service |
31306d |
======================================
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
===========
|
|
Packit Service |
31306d |
Quick Start
|
|
Packit Service |
31306d |
===========
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Coding style guidelines are about reducing the number of unnecessary
|
|
Packit Service |
31306d |
reformatting patches and making things easier for developers to work together.
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
You don't have to like them or even agree with them, but once put in place we
|
|
Packit Service |
31306d |
all have to abide by them (or vote to change them). However, coding style
|
|
Packit Service |
31306d |
should never outweigh coding itself and so the guidelines described here are
|
|
Packit Service |
31306d |
hopefully easy enough to follow as they are very common and supported by tools
|
|
Packit Service |
31306d |
and editors.
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
The basic style for C code, is the Linux kernel coding style (See
|
|
Packit Service |
31306d |
Documentation/CodingStyle in the kernel source tree). This closely matches what
|
|
Packit Service |
31306d |
libssh developers use already anyways, with a few exceptions as mentioned
|
|
Packit Service |
31306d |
below.
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
But to save you the trouble of reading the Linux kernel style guide, here
|
|
Packit Service |
31306d |
are the highlights.
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
* Maximum Line Width is 80 Characters
|
|
Packit Service |
31306d |
The reason is not about people with low-res screens but rather sticking
|
|
Packit Service |
31306d |
to 80 columns prevents you from easily nesting more than one level of
|
|
Packit Service |
31306d |
if statements or other code blocks.
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
* Use 4 Spaces to Indent
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
* No Trailing Whitespace
|
|
Packit Service |
31306d |
Clean up your files before committing.
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
* Follow the K&R guidelines. We won't go through all of them here. Do you
|
|
Packit Service |
31306d |
have a copy of "The C Programming Language" anyways right?
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
=============
|
|
Packit Service |
31306d |
Editor Hints
|
|
Packit Service |
31306d |
=============
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Emacs
|
|
Packit Service |
31306d |
------
|
|
Packit Service |
31306d |
Add the follow to your $HOME/.emacs file:
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
(add-hook 'c-mode-hook
|
|
Packit Service |
31306d |
(lambda ()
|
|
Packit Service |
31306d |
(c-set-style "linux")
|
|
Packit Service |
31306d |
(c-toggle-auto-state)))
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Vim
|
|
Packit Service |
31306d |
----
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
For the basic vi editor included with all variants of \*nix, add the
|
|
Packit Service |
31306d |
following to $HOME/.vimrc:
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
set ts=4 sw=4 et cindent
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
You can use the Vim gitmodline plugin to store this in the git config:
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
https://git.cryptomilk.org/projects/vim-gitmodeline.git/
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
For Vim, the following settings in $HOME/.vimrc will also deal with
|
|
Packit Service |
31306d |
displaying trailing whitespace:
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
if has("syntax") && (&t_Co > 2 || has("gui_running"))
|
|
Packit Service |
31306d |
syntax on
|
|
Packit Service |
31306d |
function! ActivateInvisibleCharIndicator()
|
|
Packit Service |
31306d |
syntax match TrailingSpace "[ \t]\+$" display containedin=ALL
|
|
Packit Service |
31306d |
highlight TrailingSpace ctermbg=Red
|
|
Packit Service |
31306d |
endf
|
|
Packit Service |
31306d |
autocmd BufNewFile,BufRead * call ActivateInvisibleCharIndicator()
|
|
Packit Service |
31306d |
endif
|
|
Packit Service |
31306d |
" Show tabs, trailing whitespace, and continued lines visually
|
|
Packit Service |
31306d |
set list listchars=tab:»·,trail:·,extends:…
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
" highlight overly long lines same as TODOs.
|
|
Packit Service |
31306d |
set textwidth=80
|
|
Packit Service |
31306d |
autocmd BufNewFile,BufRead *.c,*.h exec 'match Todo /\%>' . &textwidth . 'v.\+/'
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
==========================
|
|
Packit Service |
31306d |
FAQ & Statement Reference
|
|
Packit Service |
31306d |
==========================
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Comments
|
|
Packit Service |
31306d |
---------
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Comments should always use the standard C syntax. C++ style comments are not
|
|
Packit Service |
31306d |
currently allowed.
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
The lines before a comment should be empty. If the comment directly belongs to
|
|
Packit Service |
31306d |
the following code, there should be no empty line after the comment, except if
|
|
Packit Service |
31306d |
the comment contains a summary of multiple following code blocks.
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
This is good:
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
...
|
|
Packit Service |
31306d |
int i;
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
/*
|
|
Packit Service |
31306d |
* This is a multi line comment,
|
|
Packit Service |
31306d |
* which explains the logical steps we have to do:
|
|
Packit Service |
31306d |
*
|
|
Packit Service |
31306d |
* 1. We need to set i=5, because...
|
|
Packit Service |
31306d |
* 2. We need to call complex_fn1
|
|
Packit Service |
31306d |
*/
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
/* This is a one line comment about i = 5. */
|
|
Packit Service |
31306d |
i = 5;
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
/*
|
|
Packit Service |
31306d |
* This is a multi line comment,
|
|
Packit Service |
31306d |
* explaining the call to complex_fn1()
|
|
Packit Service |
31306d |
*/
|
|
Packit Service |
31306d |
ret = complex_fn1();
|
|
Packit Service |
31306d |
if (ret != 0) {
|
|
Packit Service |
31306d |
...
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
/**
|
|
Packit Service |
31306d |
* @brief This is a doxygen comment.
|
|
Packit Service |
31306d |
*
|
|
Packit Service |
31306d |
* This is a more detailed explanation of
|
|
Packit Service |
31306d |
* this simple function.
|
|
Packit Service |
31306d |
*
|
|
Packit Service |
31306d |
* @param[in] param1 The parameter value of the function.
|
|
Packit Service |
31306d |
*
|
|
Packit Service |
31306d |
* @param[out] result1 The result value of the function.
|
|
Packit Service |
31306d |
*
|
|
Packit Service |
31306d |
* @return 0 on success and -1 on error.
|
|
Packit Service |
31306d |
*/
|
|
Packit Service |
31306d |
int example(int param1, int *result1);
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
This is bad:
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
...
|
|
Packit Service |
31306d |
int i;
|
|
Packit Service |
31306d |
/*
|
|
Packit Service |
31306d |
* This is a multi line comment,
|
|
Packit Service |
31306d |
* which explains the logical steps we have to do:
|
|
Packit Service |
31306d |
*
|
|
Packit Service |
31306d |
* 1. We need to set i=5, because...
|
|
Packit Service |
31306d |
* 2. We need to call complex_fn1
|
|
Packit Service |
31306d |
*/
|
|
Packit Service |
31306d |
/* This is a one line comment about i = 5. */
|
|
Packit Service |
31306d |
i = 5;
|
|
Packit Service |
31306d |
/*
|
|
Packit Service |
31306d |
* This is a multi line comment,
|
|
Packit Service |
31306d |
* explaining the call to complex_fn1()
|
|
Packit Service |
31306d |
*/
|
|
Packit Service |
31306d |
ret = complex_fn1();
|
|
Packit Service |
31306d |
if (ret != 0) {
|
|
Packit Service |
31306d |
...
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
/*This is a one line comment.*/
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
/* This is a multi line comment,
|
|
Packit Service |
31306d |
with some more words...*/
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
/*
|
|
Packit Service |
31306d |
* This is a multi line comment,
|
|
Packit Service |
31306d |
* with some more words...*/
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Indention & Whitespace & 80 columns
|
|
Packit Service |
31306d |
------------------------------------
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
To avoid confusion, indentations have to be 4 spaces. Do not use tabs!. When
|
|
Packit Service |
31306d |
wrapping parameters for function calls, align the parameter list with the first
|
|
Packit Service |
31306d |
parameter on the previous line. For example,
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
var1 = foo(arg1,
|
|
Packit Service |
31306d |
arg2,
|
|
Packit Service |
31306d |
arg3);
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
The previous example is intended to illustrate alignment of function
|
|
Packit Service |
31306d |
parameters across lines and not as encourage for gratuitous line
|
|
Packit Service |
31306d |
splitting. Never split a line before columns 70 - 79 unless you
|
|
Packit Service |
31306d |
have a really good reason. Be smart about formatting.
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
If, switch, & Code blocks
|
|
Packit Service |
31306d |
--------------------------
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Always follow an 'if' keyword with a space but don't include additional
|
|
Packit Service |
31306d |
spaces following or preceding the parentheses in the conditional.
|
|
Packit Service |
31306d |
This is good:
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
if (x == 1)
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
This is bad:
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
if ( x == 1 )
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
or
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
if (x==1)
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Yes we have a lot of code that uses the second and third form and we are trying
|
|
Packit Service |
31306d |
to clean it up without being overly intrusive.
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Note that this is a rule about parentheses following keywords and not
|
|
Packit Service |
31306d |
functions. Don't insert a space between the name and left parentheses when
|
|
Packit Service |
31306d |
invoking functions.
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Braces for code blocks used by for, if, switch, while, do..while, etc. should
|
|
Packit Service |
31306d |
begin on the same line as the statement keyword and end on a line of their own.
|
|
Packit Service |
31306d |
You should always include braces, even if the block only contains one
|
|
Packit Service |
31306d |
statement. NOTE: Functions are different and the beginning left brace should
|
|
Packit Service |
31306d |
be located in the first column on the next line.
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
If the beginning statement has to be broken across lines due to length, the
|
|
Packit Service |
31306d |
beginning brace should be on a line of its own.
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
The exception to the ending rule is when the closing brace is followed by
|
|
Packit Service |
31306d |
another language keyword such as else or the closing while in a do..while loop.
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Good examples:
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
if (x == 1) {
|
|
Packit Service |
31306d |
printf("good\n");
|
|
Packit Service |
31306d |
}
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
for (x = 1; x < 10; x++) {
|
|
Packit Service |
31306d |
print("%d\n", x);
|
|
Packit Service |
31306d |
}
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
for (really_really_really_really_long_var_name = 0;
|
|
Packit Service |
31306d |
really_really_really_really_long_var_name < 10;
|
|
Packit Service |
31306d |
really_really_really_really_long_var_name++)
|
|
Packit Service |
31306d |
{
|
|
Packit Service |
31306d |
print("%d\n", really_really_really_really_long_var_name);
|
|
Packit Service |
31306d |
}
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
do {
|
|
Packit Service |
31306d |
printf("also good\n");
|
|
Packit Service |
31306d |
} while (1);
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Bad examples:
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
while (1)
|
|
Packit Service |
31306d |
{
|
|
Packit Service |
31306d |
print("I'm in a loop!\n"); }
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
for (x=1;
|
|
Packit Service |
31306d |
x<10;
|
|
Packit Service |
31306d |
x++)
|
|
Packit Service |
31306d |
{
|
|
Packit Service |
31306d |
print("no good\n");
|
|
Packit Service |
31306d |
}
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
if (i < 10)
|
|
Packit Service |
31306d |
print("I should be in braces.\n");
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Goto
|
|
Packit Service |
31306d |
-----
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
While many people have been academically taught that "goto"s are fundamentally
|
|
Packit Service |
31306d |
evil, they can greatly enhance readability and reduce memory leaks when used as
|
|
Packit Service |
31306d |
the single exit point from a function. But in no libssh world what so ever is a
|
|
Packit Service |
31306d |
goto outside of a function or block of code a good idea.
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Good Examples:
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
int function foo(int y)
|
|
Packit Service |
31306d |
{
|
|
Packit Service |
31306d |
int *z = NULL;
|
|
Packit Service |
31306d |
int rc = 0;
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
if (y < 10) {
|
|
Packit Service |
31306d |
z = malloc(sizeof(int)*y);
|
|
Packit Service |
31306d |
if (z == NULL) {
|
|
Packit Service |
31306d |
rc = 1;
|
|
Packit Service |
31306d |
goto done;
|
|
Packit Service |
31306d |
}
|
|
Packit Service |
31306d |
}
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
print("Allocated %d elements.\n", y);
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
done:
|
|
Packit Service |
31306d |
if (z != NULL) {
|
|
Packit Service |
31306d |
free(z);
|
|
Packit Service |
31306d |
}
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
return rc;
|
|
Packit Service |
31306d |
}
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Initialize pointers
|
|
Packit Service |
31306d |
-------------------
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
All pointer variables MUST be initialized to NULL. History has
|
|
Packit Service |
31306d |
demonstrated that uninitialized pointer variables have lead to various
|
|
Packit Service |
31306d |
bugs and security issues.
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Pointers MUST be initialized even if the assignment directly follows
|
|
Packit Service |
31306d |
the declaration, like pointer2 in the example below, because the
|
|
Packit Service |
31306d |
instructions sequence may change over time.
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Good Example:
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
char *pointer1 = NULL;
|
|
Packit Service |
31306d |
char *pointer2 = NULL;
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
pointer2 = some_func2();
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
...
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
pointer1 = some_func1();
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Typedefs
|
|
Packit Service |
31306d |
---------
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
libssh tries to avoid "typedef struct { .. } x_t;" so we do always try to use
|
|
Packit Service |
31306d |
"struct x { .. };". We know there are still such typedefs in the code, but for
|
|
Packit Service |
31306d |
new code, please don't do that anymore.
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Make use of helper variables
|
|
Packit Service |
31306d |
-----------------------------
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Please try to avoid passing function calls as function parameters in new code.
|
|
Packit Service |
31306d |
This makes the code much easier to read and it's also easier to use the "step"
|
|
Packit Service |
31306d |
command within gdb.
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Good Example:
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
char *name;
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
name = get_some_name();
|
|
Packit Service |
31306d |
if (name == NULL) {
|
|
Packit Service |
31306d |
...
|
|
Packit Service |
31306d |
}
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
rc = some_function_my_name(name);
|
|
Packit Service |
31306d |
...
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Bad Example:
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
rc = some_function_my_name(get_some_name());
|
|
Packit Service |
31306d |
...
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Please try to avoid passing function return values to if- or while-conditions.
|
|
Packit Service |
31306d |
The reason for this is better handling of code under a debugger.
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Good example:
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
x = malloc(sizeof(short) * 10);
|
|
Packit Service |
31306d |
if (x == NULL) {
|
|
Packit Service |
31306d |
fprintf(stderr, "Unable to alloc memory!\n");
|
|
Packit Service |
31306d |
}
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Bad example:
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
if ((x = malloc(sizeof(short)*10)) == NULL ) {
|
|
Packit Service |
31306d |
fprintf(stderr, "Unable to alloc memory!\n");
|
|
Packit Service |
31306d |
}
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
There are exceptions to this rule. One example is walking a data structure in
|
|
Packit Service |
31306d |
an iterator style:
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
while ((opt = poptGetNextOpt(pc)) != -1) {
|
|
Packit Service |
31306d |
... do something with opt ...
|
|
Packit Service |
31306d |
}
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
But in general, please try to avoid this pattern.
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Control-Flow changing macros
|
|
Packit Service |
31306d |
-----------------------------
|
|
Packit Service |
31306d |
|
|
Packit Service |
31306d |
Macros like STATUS_NOT_OK_RETURN that change control flow (return/goto/etc)
|
|
Packit Service |
31306d |
from within the macro are considered bad, because they look like function calls
|
|
Packit Service |
31306d |
that never change control flow. Please do not introduce them.
|