| 1 | Coding conventions in the Samba tree
|
|---|
| 2 | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
|---|
| 3 |
|
|---|
| 4 | .. contents::
|
|---|
| 5 |
|
|---|
| 6 | ===========
|
|---|
| 7 | Quick Start
|
|---|
| 8 | ===========
|
|---|
| 9 |
|
|---|
| 10 | Coding style guidelines are about reducing the number of unnecessary
|
|---|
| 11 | reformatting patches and making things easier for developers to work
|
|---|
| 12 | together.
|
|---|
| 13 | You don't have to like them or even agree with them, but once put in place
|
|---|
| 14 | we all have to abide by them (or vote to change them). However, coding
|
|---|
| 15 | style should never outweigh coding itself and so the guidelines
|
|---|
| 16 | described here are hopefully easy enough to follow as they are very
|
|---|
| 17 | common and supported by tools and editors.
|
|---|
| 18 |
|
|---|
| 19 | The basic style for C code, also mentioned in prog_guide4.txt, is the Linux kernel
|
|---|
| 20 | coding style (See Documentation/CodingStyle in the kernel source tree). This
|
|---|
| 21 | closely matches what most Samba developers use already anyways, with a few
|
|---|
| 22 | exceptions as mentioned below.
|
|---|
| 23 |
|
|---|
| 24 | The coding style for Python code is documented in PEP8,
|
|---|
| 25 | http://www.python.org/pep/pep8 (with spaces).
|
|---|
| 26 | If you have ever worked on another free software Python project, you are
|
|---|
| 27 | probably already familiar with it.
|
|---|
| 28 |
|
|---|
| 29 | But to save you the trouble of reading the Linux kernel style guide, here
|
|---|
| 30 | are the highlights.
|
|---|
| 31 |
|
|---|
| 32 | * Maximum Line Width is 80 Characters
|
|---|
| 33 | The reason is not about people with low-res screens but rather sticking
|
|---|
| 34 | to 80 columns prevents you from easily nesting more than one level of
|
|---|
| 35 | if statements or other code blocks. Use source3/script/count_80_col.pl
|
|---|
| 36 | to check your changes.
|
|---|
| 37 |
|
|---|
| 38 | * Use 8 Space Tabs to Indent
|
|---|
| 39 | No whitespace fillers.
|
|---|
| 40 |
|
|---|
| 41 | * No Trailing Whitespace
|
|---|
| 42 | Use source3/script/strip_trail_ws.pl to clean up your files before
|
|---|
| 43 | committing.
|
|---|
| 44 |
|
|---|
| 45 | * Follow the K&R guidelines. We won't go through all of them here. Do you
|
|---|
| 46 | have a copy of "The C Programming Language" anyways right? You can also use
|
|---|
| 47 | the format_indent.sh script found in source3/script/ if all else fails.
|
|---|
| 48 |
|
|---|
| 49 |
|
|---|
| 50 |
|
|---|
| 51 | ============
|
|---|
| 52 | Editor Hints
|
|---|
| 53 | ============
|
|---|
| 54 |
|
|---|
| 55 | Emacs
|
|---|
| 56 | -----
|
|---|
| 57 | Add the follow to your $HOME/.emacs file:
|
|---|
| 58 |
|
|---|
| 59 | (add-hook 'c-mode-hook
|
|---|
| 60 | (lambda ()
|
|---|
| 61 | (c-set-style "linux")
|
|---|
| 62 | (c-toggle-auto-state)))
|
|---|
| 63 |
|
|---|
| 64 |
|
|---|
| 65 | Vi
|
|---|
| 66 | --
|
|---|
| 67 | (Thanks to SATOH Fumiyasu <fumiyas@osstech.jp> for these hints):
|
|---|
| 68 |
|
|---|
| 69 | For the basic vi editor included with all variants of \*nix, add the
|
|---|
| 70 | following to $HOME/.exrc:
|
|---|
| 71 |
|
|---|
| 72 | set tabstop=8
|
|---|
| 73 | set shiftwidth=8
|
|---|
| 74 |
|
|---|
| 75 | For Vim, the following settings in $HOME/.vimrc will also deal with
|
|---|
| 76 | displaying trailing whitespace:
|
|---|
| 77 |
|
|---|
| 78 | if has("syntax") && (&t_Co > 2 || has("gui_running"))
|
|---|
| 79 | syntax on
|
|---|
| 80 | function! ActivateInvisibleCharIndicator()
|
|---|
| 81 | syntax match TrailingSpace "[ \t]\+$" display containedin=ALL
|
|---|
| 82 | highlight TrailingSpace ctermbg=Red
|
|---|
| 83 | endf
|
|---|
| 84 | autocmd BufNewFile,BufRead * call ActivateInvisibleCharIndicator()
|
|---|
| 85 | endif
|
|---|
| 86 | " Show tabs, trailing whitespace, and continued lines visually
|
|---|
| 87 | set list listchars=tab:»·,trail:·,extends:âŠ
|
|---|
| 88 |
|
|---|
| 89 | " highlight overly long lines same as TODOs.
|
|---|
| 90 | set textwidth=80
|
|---|
| 91 | autocmd BufNewFile,BufRead *.c,*.h exec 'match Todo /\%>' . &textwidth . 'v.\+/'
|
|---|
| 92 |
|
|---|
| 93 |
|
|---|
| 94 | =========================
|
|---|
| 95 | FAQ & Statement Reference
|
|---|
| 96 | =========================
|
|---|
| 97 |
|
|---|
| 98 | Comments
|
|---|
| 99 | --------
|
|---|
| 100 |
|
|---|
| 101 | Comments should always use the standard C syntax. C++
|
|---|
| 102 | style comments are not currently allowed.
|
|---|
| 103 |
|
|---|
| 104 | The lines before a comment should be empty. If the comment directly
|
|---|
| 105 | belongs to the following code, there should be no empty line
|
|---|
| 106 | after the comment, except if the comment contains a summary
|
|---|
| 107 | of multiple following code blocks.
|
|---|
| 108 |
|
|---|
| 109 | This is good:
|
|---|
| 110 |
|
|---|
| 111 | ...
|
|---|
| 112 | int i;
|
|---|
| 113 |
|
|---|
| 114 | /*
|
|---|
| 115 | * This is a multi line comment,
|
|---|
| 116 | * which explains the logical steps we have to do:
|
|---|
| 117 | *
|
|---|
| 118 | * 1. We need to set i=5, because...
|
|---|
| 119 | * 2. We need to call complex_fn1
|
|---|
| 120 | */
|
|---|
| 121 |
|
|---|
| 122 | /* This is a one line comment about i = 5. */
|
|---|
| 123 | i = 5;
|
|---|
| 124 |
|
|---|
| 125 | /*
|
|---|
| 126 | * This is a multi line comment,
|
|---|
| 127 | * explaining the call to complex_fn1()
|
|---|
| 128 | */
|
|---|
| 129 | ret = complex_fn1();
|
|---|
| 130 | if (ret != 0) {
|
|---|
| 131 | ...
|
|---|
| 132 |
|
|---|
| 133 | /**
|
|---|
| 134 | * @brief This is a doxygen comment.
|
|---|
| 135 | *
|
|---|
| 136 | * This is a more detailed explanation of
|
|---|
| 137 | * this simple function.
|
|---|
| 138 | *
|
|---|
| 139 | * @param[in] param1 The parameter value of the function.
|
|---|
| 140 | *
|
|---|
| 141 | * @param[out] result1 The result value of the function.
|
|---|
| 142 | *
|
|---|
| 143 | * @return 0 on success and -1 on error.
|
|---|
| 144 | */
|
|---|
| 145 | int example(int param1, int *result1);
|
|---|
| 146 |
|
|---|
| 147 | This is bad:
|
|---|
| 148 |
|
|---|
| 149 | ...
|
|---|
| 150 | int i;
|
|---|
| 151 | /*
|
|---|
| 152 | * This is a multi line comment,
|
|---|
| 153 | * which explains the logical steps we have to do:
|
|---|
| 154 | *
|
|---|
| 155 | * 1. We need to set i=5, because...
|
|---|
| 156 | * 2. We need to call complex_fn1
|
|---|
| 157 | */
|
|---|
| 158 | /* This is a one line comment about i = 5. */
|
|---|
| 159 | i = 5;
|
|---|
| 160 | /*
|
|---|
| 161 | * This is a multi line comment,
|
|---|
| 162 | * explaining the call to complex_fn1()
|
|---|
| 163 | */
|
|---|
| 164 | ret = complex_fn1();
|
|---|
| 165 | if (ret != 0) {
|
|---|
| 166 | ...
|
|---|
| 167 |
|
|---|
| 168 | /*This is a one line comment.*/
|
|---|
| 169 |
|
|---|
| 170 | /* This is a multi line comment,
|
|---|
| 171 | with some more words...*/
|
|---|
| 172 |
|
|---|
| 173 | /*
|
|---|
| 174 | * This is a multi line comment,
|
|---|
| 175 | * with some more words...*/
|
|---|
| 176 |
|
|---|
| 177 | Indention & Whitespace & 80 columns
|
|---|
| 178 | -----------------------------------
|
|---|
| 179 |
|
|---|
| 180 | To avoid confusion, indentations have to be tabs with length 8 (not 8
|
|---|
| 181 | ' ' characters). When wrapping parameters for function calls,
|
|---|
| 182 | align the parameter list with the first parameter on the previous line.
|
|---|
| 183 | Use tabs to get as close as possible and then fill in the final 7
|
|---|
| 184 | characters or less with whitespace. For example,
|
|---|
| 185 |
|
|---|
| 186 | var1 = foo(arg1, arg2,
|
|---|
| 187 | arg3);
|
|---|
| 188 |
|
|---|
| 189 | The previous example is intended to illustrate alignment of function
|
|---|
| 190 | parameters across lines and not as encourage for gratuitous line
|
|---|
| 191 | splitting. Never split a line before columns 70 - 79 unless you
|
|---|
| 192 | have a really good reason. Be smart about formatting.
|
|---|
| 193 |
|
|---|
| 194 |
|
|---|
| 195 | If, switch, & Code blocks
|
|---|
| 196 | -------------------------
|
|---|
| 197 |
|
|---|
| 198 | Always follow an 'if' keyword with a space but don't include additional
|
|---|
| 199 | spaces following or preceding the parentheses in the conditional.
|
|---|
| 200 | This is good:
|
|---|
| 201 |
|
|---|
| 202 | if (x == 1)
|
|---|
| 203 |
|
|---|
| 204 | This is bad:
|
|---|
| 205 |
|
|---|
| 206 | if ( x == 1 )
|
|---|
| 207 |
|
|---|
| 208 | Yes we have a lot of code that uses the second form and we are trying
|
|---|
| 209 | to clean it up without being overly intrusive.
|
|---|
| 210 |
|
|---|
| 211 | Note that this is a rule about parentheses following keywords and not
|
|---|
| 212 | functions. Don't insert a space between the name and left parentheses when
|
|---|
| 213 | invoking functions.
|
|---|
| 214 |
|
|---|
| 215 | Braces for code blocks used by for, if, switch, while, do..while, etc.
|
|---|
| 216 | should begin on the same line as the statement keyword and end on a line
|
|---|
| 217 | of their own. You should always include braces, even if the block only
|
|---|
| 218 | contains one statement. NOTE: Functions are different and the beginning left
|
|---|
| 219 | brace should be located in the first column on the next line.
|
|---|
| 220 |
|
|---|
| 221 | If the beginning statement has to be broken across lines due to length,
|
|---|
| 222 | the beginning brace should be on a line of its own.
|
|---|
| 223 |
|
|---|
| 224 | The exception to the ending rule is when the closing brace is followed by
|
|---|
| 225 | another language keyword such as else or the closing while in a do..while
|
|---|
| 226 | loop.
|
|---|
| 227 |
|
|---|
| 228 | Good examples:
|
|---|
| 229 |
|
|---|
| 230 | if (x == 1) {
|
|---|
| 231 | printf("good\n");
|
|---|
| 232 | }
|
|---|
| 233 |
|
|---|
| 234 | for (x=1; x<10; x++) {
|
|---|
| 235 | print("%d\n", x);
|
|---|
| 236 | }
|
|---|
| 237 |
|
|---|
| 238 | for (really_really_really_really_long_var_name=0;
|
|---|
| 239 | really_really_really_really_long_var_name<10;
|
|---|
| 240 | really_really_really_really_long_var_name++)
|
|---|
| 241 | {
|
|---|
| 242 | print("%d\n", really_really_really_really_long_var_name);
|
|---|
| 243 | }
|
|---|
| 244 |
|
|---|
| 245 | do {
|
|---|
| 246 | printf("also good\n");
|
|---|
| 247 | } while (1);
|
|---|
| 248 |
|
|---|
| 249 | Bad examples:
|
|---|
| 250 |
|
|---|
| 251 | while (1)
|
|---|
| 252 | {
|
|---|
| 253 | print("I'm in a loop!\n"); }
|
|---|
| 254 |
|
|---|
| 255 | for (x=1;
|
|---|
| 256 | x<10;
|
|---|
| 257 | x++)
|
|---|
| 258 | {
|
|---|
| 259 | print("no good\n");
|
|---|
| 260 | }
|
|---|
| 261 |
|
|---|
| 262 | if (i < 10)
|
|---|
| 263 | print("I should be in braces.\n");
|
|---|
| 264 |
|
|---|
| 265 |
|
|---|
| 266 | Goto
|
|---|
| 267 | ----
|
|---|
| 268 |
|
|---|
| 269 | While many people have been academically taught that "goto"s are
|
|---|
| 270 | fundamentally evil, they can greatly enhance readability and reduce memory
|
|---|
| 271 | leaks when used as the single exit point from a function. But in no Samba
|
|---|
| 272 | world what so ever is a goto outside of a function or block of code a good
|
|---|
| 273 | idea.
|
|---|
| 274 |
|
|---|
| 275 | Good Examples:
|
|---|
| 276 |
|
|---|
| 277 | int function foo(int y)
|
|---|
| 278 | {
|
|---|
| 279 | int *z = NULL;
|
|---|
| 280 | int ret = 0;
|
|---|
| 281 |
|
|---|
| 282 | if (y < 10) {
|
|---|
| 283 | z = malloc(sizeof(int) * y);
|
|---|
| 284 | if (z == NULL) {
|
|---|
| 285 | ret = 1;
|
|---|
| 286 | goto done;
|
|---|
| 287 | }
|
|---|
| 288 | }
|
|---|
| 289 |
|
|---|
| 290 | print("Allocated %d elements.\n", y);
|
|---|
| 291 |
|
|---|
| 292 | done:
|
|---|
| 293 | if (z != NULL) {
|
|---|
| 294 | free(z);
|
|---|
| 295 | }
|
|---|
| 296 |
|
|---|
| 297 | return ret;
|
|---|
| 298 | }
|
|---|
| 299 |
|
|---|
| 300 |
|
|---|
| 301 | Primitive Data Types
|
|---|
| 302 | --------------------
|
|---|
| 303 |
|
|---|
| 304 | Samba has large amounts of historical code which makes use of data types
|
|---|
| 305 | commonly supported by the C99 standard. However, at the time such types
|
|---|
| 306 | as boolean and exact width integers did not exist and Samba developers
|
|---|
| 307 | were forced to provide their own. Now that these types are guaranteed to
|
|---|
| 308 | be available either as part of the compiler C99 support or from
|
|---|
| 309 | lib/replace/, new code should adhere to the following conventions:
|
|---|
| 310 |
|
|---|
| 311 | * Booleans are of type "bool" (not BOOL)
|
|---|
| 312 | * Boolean values are "true" and "false" (not True or False)
|
|---|
| 313 | * Exact width integers are of type [u]int[8|16|32|64]_t
|
|---|
| 314 |
|
|---|
| 315 |
|
|---|
| 316 | Typedefs
|
|---|
| 317 | --------
|
|---|
| 318 |
|
|---|
| 319 | Samba tries to avoid "typedef struct { .. } x_t;" so we do always try to use
|
|---|
| 320 | "struct x { .. };". We know there are still such typedefs in the code,
|
|---|
| 321 | but for new code, please don't do that anymore.
|
|---|
| 322 |
|
|---|
| 323 | Initialize pointers
|
|---|
| 324 | -------------------
|
|---|
| 325 |
|
|---|
| 326 | All pointer variables MUST be initialized to NULL. History has
|
|---|
| 327 | demonstrated that uninitialized pointer variables have lead to various
|
|---|
| 328 | bugs and security issues.
|
|---|
| 329 |
|
|---|
| 330 | Pointers MUST be initialized even if the assignment directly follows
|
|---|
| 331 | the declaration, like pointer2 in the example below, because the
|
|---|
| 332 | instructions sequence may change over time.
|
|---|
| 333 |
|
|---|
| 334 | Good Example:
|
|---|
| 335 |
|
|---|
| 336 | char *pointer1 = NULL;
|
|---|
| 337 | char *pointer2 = NULL;
|
|---|
| 338 |
|
|---|
| 339 | pointer2 = some_func2();
|
|---|
| 340 |
|
|---|
| 341 | ...
|
|---|
| 342 |
|
|---|
| 343 | pointer1 = some_func1();
|
|---|
| 344 |
|
|---|
| 345 | Bad Example:
|
|---|
| 346 |
|
|---|
| 347 | char *pointer1;
|
|---|
| 348 | char *pointer2;
|
|---|
| 349 |
|
|---|
| 350 | pointer2 = some_func2();
|
|---|
| 351 |
|
|---|
| 352 | ...
|
|---|
| 353 |
|
|---|
| 354 | pointer1 = some_func1();
|
|---|
| 355 |
|
|---|
| 356 | Make use of helper variables
|
|---|
| 357 | ----------------------------
|
|---|
| 358 |
|
|---|
| 359 | Please try to avoid passing function calls as function parameters
|
|---|
| 360 | in new code. This makes the code much easier to read and
|
|---|
| 361 | it's also easier to use the "step" command within gdb.
|
|---|
| 362 |
|
|---|
| 363 | Good Example:
|
|---|
| 364 |
|
|---|
| 365 | char *name = NULL;
|
|---|
| 366 |
|
|---|
| 367 | name = get_some_name();
|
|---|
| 368 | if (name == NULL) {
|
|---|
| 369 | ...
|
|---|
| 370 | }
|
|---|
| 371 |
|
|---|
| 372 | ret = some_function_my_name(name);
|
|---|
| 373 | ...
|
|---|
| 374 |
|
|---|
| 375 |
|
|---|
| 376 | Bad Example:
|
|---|
| 377 |
|
|---|
| 378 | ret = some_function_my_name(get_some_name());
|
|---|
| 379 | ...
|
|---|
| 380 |
|
|---|
| 381 | Please try to avoid passing function return values to if- or
|
|---|
| 382 | while-conditions. The reason for this is better handling of code under a
|
|---|
| 383 | debugger.
|
|---|
| 384 |
|
|---|
| 385 | Good example:
|
|---|
| 386 |
|
|---|
| 387 | x = malloc(sizeof(short)*10);
|
|---|
| 388 | if (x == NULL) {
|
|---|
| 389 | fprintf(stderr, "Unable to alloc memory!\n");
|
|---|
| 390 | }
|
|---|
| 391 |
|
|---|
| 392 | Bad example:
|
|---|
| 393 |
|
|---|
| 394 | if ((x = malloc(sizeof(short)*10)) == NULL ) {
|
|---|
| 395 | fprintf(stderr, "Unable to alloc memory!\n");
|
|---|
| 396 | }
|
|---|
| 397 |
|
|---|
| 398 | There are exceptions to this rule. One example is walking a data structure in
|
|---|
| 399 | an iterator style:
|
|---|
| 400 |
|
|---|
| 401 | while ((opt = poptGetNextOpt(pc)) != -1) {
|
|---|
| 402 | ... do something with opt ...
|
|---|
| 403 | }
|
|---|
| 404 |
|
|---|
| 405 | But in general, please try to avoid this pattern.
|
|---|
| 406 |
|
|---|
| 407 |
|
|---|
| 408 | Control-Flow changing macros
|
|---|
| 409 | ----------------------------
|
|---|
| 410 |
|
|---|
| 411 | Macros like NT_STATUS_NOT_OK_RETURN that change control flow
|
|---|
| 412 | (return/goto/etc) from within the macro are considered bad, because
|
|---|
| 413 | they look like function calls that never change control flow. Please
|
|---|
| 414 | do not use them in new code.
|
|---|
| 415 |
|
|---|
| 416 | The only exception is the test code that depends repeated use of calls
|
|---|
| 417 | like CHECK_STATUS, CHECK_VAL and others.
|
|---|
| 418 |
|
|---|
| 419 |
|
|---|
| 420 | DEBUG statements
|
|---|
| 421 | ----------------
|
|---|
| 422 |
|
|---|
| 423 | Use these following macros instead of DEBUG:
|
|---|
| 424 |
|
|---|
| 425 | DBG_ERR log level 0 error conditions
|
|---|
| 426 | DBG_WARNING log level 1 warning conditions
|
|---|
| 427 | DBG_NOTICE log level 3 normal, but significant, condition
|
|---|
| 428 | DBG_INFO log level 5 informational message
|
|---|
| 429 | DBG_DEBUG log level 10 debug-level message
|
|---|
| 430 |
|
|---|
| 431 | Example usage:
|
|---|
| 432 |
|
|---|
| 433 | DBG_ERR("Memory allocation failed\n");
|
|---|
| 434 | DBG_DEBUG("Received %d bytes\n", count);
|
|---|
| 435 |
|
|---|
| 436 | The messages from these macros are automatically prefixed with the
|
|---|
| 437 | function name.
|
|---|