Skip to content
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

coding: Update coding guidelines #149

Merged
merged 1 commit into from
Aug 4, 2023
Merged

coding: Update coding guidelines #149

merged 1 commit into from
Aug 4, 2023

Conversation

agkaminski
Copy link
Member

JIRA: RTOS-490

@anglov @nalajcie I am open to critique and suggestions. Have I forgot something?

Copy link
Member

@nalajcie nalajcie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

H, many thanks for the docs update. I've pointed out some small typos (we really need spelling and markdown linting GA here!). As for the contents:

  • I would highlight that we're using C99 standard as a base
  • regarding the local variables in kernel - I think we should straighten the "according to C89 standard" (it says that the variables need to be declared at the beginning of each of the compound statement - so after each {) - either dropping the C89 standard or allowing declaring variables in function sub-scopes
  • somewhere near limiting the scope of the variable I would advice also not to reuse variables for different purposes across the function
  • not sure if this should be a rule but I would write that the variables (especially of complex types) need to be fully initialized before being used (or re-used in case of custom allocators) - partial initialization is not allowed (eg. partial msg_t initialization might lead to ugly inconsistent errors). For complex not zero-initialized variables I would advice using designated initializers

coding.md Outdated Show resolved Hide resolved
coding.md Outdated Show resolved Hide resolved
coding.md Outdated Show resolved Hide resolved
coding.md Show resolved Hide resolved
coding.md Outdated Show resolved Hide resolved
Copy link
Member

@anglov anglov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional remark, not covered with the changes:

About function names:
in terms of (non-standard) libraries, additional unique prefix should be used to avoid symbol colision
In our corelibs I can type one example of not unique and not obvious prefix: in libgraph header soft.h contains prefix soft_
soft can meaning many things and name conflict can be provoked accidentally

coding.md Show resolved Hide resolved
coding.md Show resolved Hide resolved
coding.md Outdated Show resolved Hide resolved
@agkaminski agkaminski changed the title conding: Update coding guildelines coding: Update coding guildelines Jun 16, 2023
coding.md Outdated Show resolved Hide resolved
coding.md Show resolved Hide resolved
coding.md Show resolved Hide resolved
coding.md Show resolved Hide resolved
coding.md Outdated Show resolved Hide resolved
coding.md Show resolved Hide resolved
coding.md Show resolved Hide resolved
coding.md Outdated Show resolved Hide resolved
coding.md Show resolved Hide resolved
coding.md Show resolved Hide resolved
coding.md Show resolved Hide resolved
coding.md Outdated Show resolved Hide resolved
coding.md Show resolved Hide resolved
@agkaminski
Copy link
Member Author

@anglov I think this rule is presented in Function names chapter

@anglov
Copy link
Member

anglov commented Jun 16, 2023

@anglov I think this rule is presented in Function names chapter

Yep, I write about things not covered by this chapter.

In general - it's common good practise to have unique prefixes inside non-standard libraries in C. In C++ this things are covered by namespaces, in C we only can rely on function names.

I believe that rule in coding standard is good for application code. For non-standard libraries it should be like:

[_]<library_prefix>_<subsystem>_<functionality>
or
[_]<library_prefix>_<functionality> when it's simple library or it's good to not have functionality prefix (eg. in main module)

Library prefix should be unique, and traceable.

I know that it's suggestion of kinda new rule but I believe not including it will cause a major problems sooner or later.

(btw. - for future consideration is using _ as prefix is also controvelsial with C/POSIX)

@agkaminski agkaminski changed the title coding: Update coding guildelines coding: Update coding guidelines Jun 19, 2023
@agkaminski agkaminski force-pushed the agkaminski/rtos-490 branch 2 times, most recently from 4164e2a to 7fa9323 Compare June 19, 2023 14:27
coding.md Show resolved Hide resolved
coding.md Show resolved Hide resolved
@agkaminski
Copy link
Member Author

@anglov So, I added a rule about libraries with submodules, but I don't think I like it tbh.
How about instead of int foo_bar_start(void) make int foo_barStart(void)?
There're examples of both of these styles (+ many examples of no lib prefix at all), so we're free to choose.

@anglov
Copy link
Member

anglov commented Jun 19, 2023

@agkaminski well, to be honest the most important is to create that rule and use it consistently.

Personally I like the version with foo_bar_someNeatFunction (at least as a option) as it allows to split large libs to modules with separate visible namespace, similar as in apps.

But if you think that it's too much in naming, then I will be ok with common approach with one prefix across the lib.

coding.md Outdated Show resolved Hide resolved
## Global variables

Global variables should be used only if they're absolutely necessary. You should avoid using globally initialized variables. If they are used, global variables can only be placed in common structures. The structure should be named after the system module that implements it, followed by _common. Example notation is shown below.
In the kernel code global variables should be always initialized in runtime. Global variables should be used only if they're absolutely necessary. Scope should be limited whenever possible by using `static`. If they are used, global variables can only be placed in common structures. The structure should be named after the system module that implements it, followed by `_common`. Example notation is shown below.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[markdownlint] reported by reviewdog 🐶
MD013/line-length Line length [Expected: 120; Actual: 413]

spinlock_t spinlock;
} pmap_common;
```

It is acceptable to omit module name in user space applications (i.e. not in the kernel) and name the structure `common`, only if static is used.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[markdownlint] reported by reviewdog 🐶
MD013/line-length Line length [Expected: 120; Actual: 145]

@agkaminski agkaminski requested a review from anglov August 3, 2023 15:28
Copy link
Member

@anglov anglov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm more than happy with current version

@agkaminski agkaminski merged commit 42c1aa9 into master Aug 4, 2023
2 of 3 checks passed
@agkaminski agkaminski deleted the agkaminski/rtos-490 branch August 4, 2023 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants