-
Notifications
You must be signed in to change notification settings - Fork 11
Coding Standard #240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Coding Standard #240
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
markdownlint
[markdownlint] reported by reviewdog 🐶
MD013/line-length Line length [Expected: 120; Actual: 222]
phoenix-rtos-doc/coding/misracprofile.md
Line 108 in 4df18e1
| | Rule 11.3 | Required | DISABLED | NO | This rule is not applicable for the Phoenix-RTOS kernel source code, because casting between different types of pointers can not be avoided in the operating system implementation. | |
[markdownlint] reported by reviewdog 🐶
MD013/line-length Line length [Expected: 120; Actual: 234]
phoenix-rtos-doc/coding/misracprofile.md
Line 111 in 4df18e1
| | Rule 11.6 | Required | DISABLED | NO | This rule is not applicable for the Phoenix-RTOS kernel source code, because casting between pointer to void and an arithmetic type can not be avoided inn the operating system implementation. | |
[markdownlint] reported by reviewdog 🐶
MD013/line-length Line length [Expected: 120; Actual: 167]
phoenix-rtos-doc/coding/misracprofile.md
Line 185 in 4df18e1
| | Rule 21.1 | Required | DISABLED | NO | This rule is not applicable for Phoenix-RTOS kernel source code, because common identifiers are defined within its codebase. | |
[markdownlint] reported by reviewdog 🐶
MD013/line-length Line length [Expected: 120; Actual: 167]
phoenix-rtos-doc/coding/misracprofile.md
Line 186 in 4df18e1
| | Rule 21.2 | Required | DISABLED | NO | This rule is not applicable for Phoenix-RTOS kernel source code, because common identifiers are defined within its codebase. | |
[markdownlint] reported by reviewdog 🐶
MD013/line-length Line length [Expected: 120; Actual: 187]
phoenix-rtos-doc/coding/misracprofile.md
Line 189 in 4df18e1
| | Rule 21.5 | Required | DISABLED | NO | This rule is not applicable for the Phoenix-RTOS kernel source code, because <signal.h> is proprietary to its codebase and has defined behavior. | |
[markdownlint] reported by reviewdog 🐶
MD013/line-length Line length [Expected: 120; Actual: 203]
phoenix-rtos-doc/coding/misracprofile.md
Line 194 in 4df18e1
| | Rule 21.10 | Required | DISABLED | NO | This rule is not applicable for the Phoenix-RTOS kernel source code, because date and time functions are proprietary to its codebase and has defined behavior. | |
|
We should probably enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
markdownlint
[markdownlint] reported by reviewdog 🐶
MD013/line-length Line length [Expected: 120; Actual: 167]
phoenix-rtos-doc/coding/misracprofile.md
Line 185 in 33ff070
| | Rule 21.1 | Required | DISABLED | NO | This rule is not applicable for Phoenix-RTOS kernel source code, because common identifiers are defined within its codebase. | |
[markdownlint] reported by reviewdog 🐶
MD013/line-length Line length [Expected: 120; Actual: 167]
phoenix-rtos-doc/coding/misracprofile.md
Line 186 in 33ff070
| | Rule 21.2 | Required | DISABLED | NO | This rule is not applicable for Phoenix-RTOS kernel source code, because common identifiers are defined within its codebase. | |
[markdownlint] reported by reviewdog 🐶
MD013/line-length Line length [Expected: 120; Actual: 187]
phoenix-rtos-doc/coding/misracprofile.md
Line 189 in 33ff070
| | Rule 21.5 | Required | DISABLED | NO | This rule is not applicable for the Phoenix-RTOS kernel source code, because <signal.h> is proprietary to its codebase and has defined behavior. | |
[markdownlint] reported by reviewdog 🐶
MD013/line-length Line length [Expected: 120; Actual: 203]
phoenix-rtos-doc/coding/misracprofile.md
Line 194 in 33ff070
| | Rule 21.10 | Required | DISABLED | NO | This rule is not applicable for the Phoenix-RTOS kernel source code, because date and time functions are proprietary to its codebase and has defined behavior. | |
julianuziemblo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
English spelling/grammar/logical nitpicks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
markdownlint
[markdownlint] reported by reviewdog 🐶
MD024/no-duplicate-heading Multiple headings with the same content [Context: "#### Example"]
phoenix-rtos-doc/coding/index.md
Line 716 in 43c5a58
| #### Example |
[markdownlint] reported by reviewdog 🐶
MD024/no-duplicate-heading Multiple headings with the same content [Context: "#### Example"]
phoenix-rtos-doc/coding/index.md
Line 728 in 43c5a58
| #### Example |
[markdownlint] reported by reviewdog 🐶
MD024/no-duplicate-heading Multiple headings with the same content [Context: "### PS-CODE-069"]
phoenix-rtos-doc/coding/index.md
Line 751 in 43c5a58
| ### PS-CODE-069 |
[markdownlint] reported by reviewdog 🐶
MD024/no-duplicate-heading Multiple headings with the same content [Context: "#### Example"]
phoenix-rtos-doc/coding/index.md
Line 755 in 43c5a58
| #### Example |
[markdownlint] reported by reviewdog 🐶
MD024/no-duplicate-heading Multiple headings with the same content [Context: "#### Note"]
phoenix-rtos-doc/coding/index.md
Line 779 in 43c5a58
| #### Note |
5a555d6 to
0720e81
Compare
9066de6 to
72eb79b
Compare
| #### Note | ||
|
|
||
| The use of preprocessing conditionals makes it harder to follow the code logic and can be used only | ||
| if it is absolutely necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could provide an example when it could be used, like defining common functionality for different platforms or running code only in debug mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide complete list of such exceptions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example provided could be connected to the example for the next rule: having code for MMU and no-MMU architectures in the same codebase and conditionally compiling it when necessary - this way we maximize code reuse
| If disabled or dead code is present (it was necessary), preprocessor should be used accordingly: | ||
|
|
||
| ```c | ||
| releveantCode(); | ||
|
|
||
| #if 0 | ||
| obsoleteFunction(); | ||
| #endif | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was under the impression we didn't want code like this in the kernel as we removed such situations, relying on version control to keep replaced code accessible.
| It is not allowed to make an indentation with space character. The source code | ||
| used for development tests (e.g. printf debug) should be entered without indentation. The following code presents | ||
| correctly formatted code with one line (`lib_printf`) entered for debug purposes. The inserted line should be removed | ||
| in the final code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, we should remove the not-indented debug messages - they should never appear in commited code anyway.
Please note that the example code currently breaks this rule (the debug code is indented) - in current coding convention it's written correctly, so I assume it's just a ^C^V error.
| @@ -1,14 +1,49 @@ | |||
| # Coding convention | |||
| # Software Code Standards | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this standard is only for C language, should we emphasize it somewhere? (it doesn't speak about eg. Assembler)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, we are missing Assembler part. Can you point me to any resources I can use to formulate rules for Assembler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really, what I'm saying is that we should treat this document as C Software Coding Standard (as when you're writing in bash, make or some other lang used withing project (either code or configuration) - the rules to follow are not included in this document (and should not be - there should be separate document per language - together they create Software Code Standards)).
| ### PS-CODE-REC-007 | ||
|
|
||
| Labels in each file should be constructed according to presented rules. Modification of these rules is not allowed. | ||
| Only characters from the standard ASCII range (0x20–0x7E) should be used in source files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turbo nit: what about new line etc? 😄
| ### PS-CODE-REC-012 | ||
| Functions should be short and not too complex in terms of logic. The function should do one thing only. Functions should | ||
| be separated with two newline characters. | ||
| The function should do one thing only. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really followed and not really practical rule, I vote against
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it was a rule yeah, but it's a recommendation - IMO valueable to have some form of it here
| is the name of subsystem or file to which function belongs and `<functionality>` is the brief sentence explaining the | ||
| implemented functionality. The subsystem name should be a one word without the underline characters. The functionality | ||
| could be expressed using many words but without the underlines. In such case camelCase should be used. | ||
| Functions should be separated with two `newline` characters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 empty lines instead of newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kind of depends how specific we have to get with these rules. Maybe newlines is better for when someone uses Windows for development and pollutes the repo with CRLFs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thinking, UNIX style new line should be enforced
| #### Note | ||
| An underscore character at the start of the function name indicates that the function is not synchronized, and its usage | ||
| by two parallel threads requires external synchronization. A good example of such function is the internal function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| by two parallel threads requires external synchronization. A good example of such function is the internal function | |
| requires external synchronization. A good example of such function is the internal function |
| ### PS-CODE-REC-020 | ||
| Functions exported outside the subsystem should be named with the subsystem name only. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We require such naming scheme anyway
|
|
||
| ### PS-CODE-REC-55 | ||
|
|
||
| The type specifiers from the list bellow should be used: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about stdint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like "don't write unsigned instead of unsigned int because this is our standard" - this should definetely be re-worded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe something like "If default C types are to be used, only use those from the list below"?
|
|
||
| *Recommendation derived from MISRA C.* | ||
|
|
||
| Types of defined length should be used over default C types. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in direct contradiction with PS-CODE-REC-55
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd make this recommendation preceed PS-CODE-REC-55
|
|
||
| ### PS-CODE-REC-060 | ||
|
|
||
| `//` comments should not to be used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C++ style
| #### Note | ||
|
|
||
| Comments are not the place to express programmer's novel writing skills. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's tone down aggression while we're here
| #### Note | |
| Comments are not the place to express programmer's novel writing skills. |
| #### Explanation | ||
|
|
||
| It’s not sufficient to mark them with `U` as MISRA classifies `U` constants, despite the C standard implications, as | ||
| 8-bit essential type (see the “To assist in understanding” example in Rule 12.2 rationale, D.3 and D.7.4). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot assume that reader has an access to the closed norm.
If it would happen that we want to assume that, then exact document should be also specified (what if newer or older revision place the rule and directive on some other identifier?)
Coding guide is converted to Coding Standard with additional rules added to comply with MISRA C rules.