Skip to content

Commit

Permalink
CONTRIBUTING.md: improve coding-style related sections
Browse files Browse the repository at this point in the history
This is highlight that code readability is the real goal of all the
coding-style rules. We should not do coding-style just for coding-style,
e.g. when clang-format suggests crazy formating we should not follow it
if we feel it is bad.

Signed-off-by: Pavel Tikhomirov <[email protected]>
  • Loading branch information
Snorch committed Aug 29, 2023
1 parent 19c7703 commit 05770aa
Showing 1 changed file with 43 additions and 3 deletions.
46 changes: 43 additions & 3 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,16 @@ This should create the `./criu/criu` executable.

When you change the source code, please keep in mind the following code conventions:

* code is written to be read, so the code readability is the most important thing you need to have in mind when preparing patches
* we prefer tabs and indentations to be 8 characters width
* CRIU mostly follows [Linux kernel coding style](https://www.kernel.org/doc/Documentation/process/coding-style.rst), but we are less strict than the kernel community.
* we prefer line length of 80 characters or less, more is allowed if it helps with code readability
* CRIU mostly follows [Linux kernel coding style](https://www.kernel.org/doc/Documentation/process/coding-style.rst), but we are less strict than the kernel community

Other conventions can be learned from the source code itself. In short, make sure your new code
looks similar to what is already there.
Other conventions can be learned from the source code itself. In short, make sure your new code looks similar to what is already there.

## Automatic tools to fix coding-style

Important: Tools for automatic indentation are there to advise you, but should not be considered as a "source of truth", as tools also make nasty mistakes from time to time which completely break code readability.

The following command can be used to automatically run a code linter for Python files (flake8), Shell scripts (shellcheck),
text spelling (codespell), and a number of CRIU-specific checks (usage of print macros and EOL whitespace for C files).
Expand Down Expand Up @@ -84,6 +89,41 @@ to check the last *N* commits for formatting errors, without applying the change
Note that for pull requests, the "Run code linter" workflow runs these checks for all commits. If a clang-format error is detected
we need to review the suggested changes and decide if they should be fixed before merging.

Here are some bad examples of clang-format-ing:

* if clang-format tries to force 120 characters and breaks readability - it is wrong:

```
@@ -58,8 +59,7 @@ static int register_membarriers(void)
}
if (!all_ok) {
- fail("can't register membarrier()s - tried %#x, kernel %#x",
- barriers_registered, barriers_supported);
+ fail("can't register membarrier()s - tried %#x, kernel %#x", barriers_registered, barriers_supported);
return -1;
}
```

* if clang-format breaks your beautiful readability friendly alignments in structures, comments or defines - it is wrong:

```
--- a/test/zdtm/static/membarrier.c
+++ b/test/zdtm/static/membarrier.c
@@ -27,9 +27,10 @@ static const struct {
int register_cmd;
int execute_cmd;
} membarrier_cmds[] = {
- { "", MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED, MEMBARRIER_CMD_PRIVATE_EXPEDITED },
- { "_SYNC_CORE", MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE, MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE },
- { "_RSEQ", MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ, MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ },
+ { "", MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED, MEMBARRIER_CMD_PRIVATE_EXPEDITED },
+ { "_SYNC_CORE", MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE,
+ MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE },
+ { "_RSEQ", MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ, MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ },
};
```

## Test your changes

CRIU comes with an extensive test suite. To check whether your changes introduce any regressions, run
Expand Down

0 comments on commit 05770aa

Please sign in to comment.