gh-1031 refactor C Style Guide #1692
Conversation
Onvember
force-pushed
the
onvember/gh-1031
branch
from
62268e9 to
77a7b8c
Compare
doc/dev_guide/c_style_guide_new.rst
Outdated
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| Tabs are 8 characters, and thus indentations are | ||
| also 8 characters. There are heretic movements that try to make indentations |
There was a problem hiding this comment.
It is worth mentioning that we use 8-width tabs, not 8 whitespaces.
doc/dev_guide/c_style_guide_new.rst
Outdated
| if (condition) do_this; | ||
| do_something_everytime; | ||
|
|
||
| Don't put multiple assignments on a single line either. Kernel coding style |
There was a problem hiding this comment.
It is not Kernel coding style anymore. We must purge all mentioning of the kernel from the doc.
doc/dev_guide/c_style_guide_new.rst
Outdated
| Lots of people think that typedefs ``help readability``. Not so. They are | ||
| useful only for: | ||
|
|
||
| #. totally opaque objects (where the typedef is actively used to **hide** |
There was a problem hiding this comment.
Totally should start with a capital letter?
doc/dev_guide/c_style_guide_new.rst
Outdated
| might be an ``unsigned int`` and under other configurations might be | ||
| ``unsigned long``, then by all means go ahead and use a typedef. | ||
|
|
||
| #. when you use sparse to literally create a **new** type for |
doc/dev_guide/c_style_guide_new.rst
Outdated
| brain to become accustomed to the standard types like ``uint32_t``, | ||
| some people object to their use anyway. | ||
|
|
||
| Therefore, the Linux-specific ``u8/u16/u32/u64`` types and their |
There was a problem hiding this comment.
We don't use these types. They can only be found in old SQL code, but we are getting rid of them.
doc/dev_guide/c_style_guide_new.rst
Outdated
| a parameter is known to be a compiletime constant, and as a result of this | ||
| constantness you *know* the compiler will be able to optimize most of your | ||
| function away at compile time. For a good example of this later case, see | ||
| the kmalloc() inline function. |
There was a problem hiding this comment.
kmalloc() is a kernel function, we should not mention it.
doc/dev_guide/c_style_guide_new.rst
Outdated
| than an indication of whether the computation succeeded, are not subject to | ||
| this rule. Generally they indicate failure by returning some out-of-range | ||
| result. Typical examples would be functions that return pointers; they use | ||
| NULL or the ERR_PTR mechanism to report failure. |
There was a problem hiding this comment.
We don't have ERR_PTR, only NULL.
doc/dev_guide/c_style_guide_new.rst
Outdated
|
|
||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
| Chapter 14: Inline assembly | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
There was a problem hiding this comment.
Tbh, I would drop chapter this entirely. We don't write in assembly except extremely rare exceptions which I can't even remember what was the last one.
doc/dev_guide/c_style_guide_new.rst
Outdated
| Prefer to compile out entire functions, rather than portions of functions or | ||
| portions of expressions. Rather than putting an ``#ifdef`` in an expression, | ||
| factor out part or all of the expression into a separate helper function and | ||
| apply the conditional to that function. |
There was a problem hiding this comment.
Shouldn't it be "the conditional" -> "the condition"?
doc/dev_guide/c_style_guide_new.rst
Outdated
| going unused, do not compile it and use #if for this. | ||
| (However, if a function or variable *always* goes unused, delete it.) | ||
|
|
||
| Within code, where possible, use the IS_ENABLED macro to convert a Kconfig |
There was a problem hiding this comment.
We don't have IS_ENABLED nor Kconfig. I would propose to drop the text from here to line 841.
Onvember
force-pushed
the
onvember/gh-1031
branch
from
7587e9e to
27d69ed
Compare
The current page about C style is quite old and confusing so I've tried to make it better.
Tarantool style features will be rendered like this:
!!!NOTE FOR REVIEWERS!!!
The "c_style_guide.rst" and "c_style_guide_new.rst" files are identical. I added a "c_style_guide_new.rst" just to make sure that git comparisons with the old version would not get in the way during the review. I will remove extra files after review. Feel free to comment on any of these files.
fixes #1031