Skip to content

Changes

Page history
Editorial clean-up, added TOC, removed BIND10 references and deprecated rules, unified formatting authored by Tomek Mrugalski's avatar Tomek Mrugalski
Hide whitespace changes
Inline Side-by-side
Processes/coding-guidelines.md
View page @ c3ccf58c
This page documents some coding guidelines for Kea project.
This page documents some coding guidelines for the Kea project. See [Stork coding guilelines](https://gitlab.isc.org/isc-projects/stork/-/wikis/Processes/coding-guidelines) for UI related guidelines. Also refer to the [BIND 9 coding guidelines](https://gitlab.isc.org/isc-projects/bind9/-/blob/main/doc/dev/style.md).
see [Stork coding guilelines](https://gitlab.isc.org/isc-projects/stork/-/wikis/Processes/coding-guidelines) for UI related guidelines. Also refer to the [BIND 9 coding guidelines](https://gitlab.isc.org/isc-projects/bind9/-/blob/main/doc/dev/style.md).
[[_TOC_]]
# General rules
Those apply to all code in all languages.
# Common
## Testing and Documentation addresses and prefixes
<details>
<summary>Testing/Documentation addresses and prefixes</summary>
Use 192.0.2.0/24 and 2001:db8::/32 for purposes like addresses used in test cases or examples in documentation (source: [RFC5737](https://datatracker.ietf.org/doc/html/rfc5737) and [RFC3849](https://datatracker.ietf.org/doc/html/rfc3849)). Likewise, use reserved example domain names such as example.com, .test, .example, etc for domain names used in these cases. They are reserved by specifications and should be the safest in terms of collision avoidance.
## Dead code
Use 192.0.2.0/24 and 2001:db8::/32 for purposes like addresses used in test cases or examples in documentation. Likewise, use reserved example domain names such as example.com, .test, .example, etc for domain names used in these cases. They are reserved by specifications and should be the safest in terms of collision avoidance.
Dead code is bad; it suffers from code rot, and it looks unclean. There are some circumstances where there is a reason to keep a bit of unused code around for a while, but these should be the exception rather than the rule, and it should be very clear why it is there, and on what conditions and when it will be re-enabled or removed completely.
</details>
<details>
<summary> TODO Comments </summary>
Any dead code (both files that are unused and blocks of commented-out code) should in principle be removed. If there is a very good reason to keep it around for a while, it must be accompanied by a comment explaining why it is still there, and when it will be removed or enabled again. This comment should point to a ticket so that we do not forget about it.
We sprinkle comments in code with keywords to indicate pending work.
## Line length
In Kea, @todo is preferred. It should be prepended with triple ///, so it will show up on a nicely auto-generated Doxygen todo list. If there is a corresponding ticket, feel free to specify its number in the comment. Unless other wise specified, issue #1234 means a ticket in the trac, available at http://kea.isc.org.
</details>
Source code not exceeding 80 columns is preferred. This is derived from
the [wiki:BIND9CodingGuidelines BIND 9 coding guidelines], mainly for
style consistency. However, C++ names (especially with namespaces) are significantly
longer, thus strictly adhering to this causes overly wrapped code that is both
ugly and difficult to read. Therefore it is permitted to use lines up to 100 columns long.
## Dead code
Rationale: It is 2021 and all ISC employees have equipment that is at most 3 years old. You should have FullHD (or better) display available to you. You should be able to display two files side by side comfortably. If this is a problem, get a larger display.
Dead code is bad; it suffers from code rot, and it looks unclean. There are some circumstances where there is a reason to keep a bit of unused code around for a while, but these should be the exception rather than the rule, and it should be very clear why it is there, and on what conditions and when it will be re-enabled or removed completely.
## Tabs & Indentation
Any dead code (both files that are unused and blocks of commented-out code) should in principle be removed. If there is a very good reason to keep it around for a while, it must be accompanied by a comment explaining why it is still there, and when it will be removed or enabled again. This comment should point to a ticket so that we do not forget about it.
Do not use hard tabs.
Indentation at each level is 4 spaces for C++, other languages should use what is "usual and expected."
In C++ we use the BSD style (also from BIND 9), where continuing lines are aligned with the corresponding opening parenthesis, like this:
```cpp
if (JS_DefineProperty(cx, o, "data",
STRING_TO_JSVAL(JS_NewStringCopyN(cx, data, res)),
0, 0, JSPROP_ENUMERATE) != 0) {
```
# Python Style
We don't use python code anymore in the core code. If you found any leftovers, feel free to remove them.
......@@ -34,8 +47,9 @@ We don't use python code anymore in the core code. If you found any leftovers, f
# C++ Style
<details>
<summary> File Name Conventions </summary>
We use C++11 and in general all features available in the language can be used. However, when introducing more exotic constructs, make sure it does not break on older, but still popular platforms. CentOS and RHEL are particularly known for having stable, but old versions of everything, including compilers and libraries.
## File Name Conventions
Use .cc for C++ source files. This is basically a mere preference and to ensure consistency throughout the package.
......@@ -44,7 +58,6 @@ Use .h for C++ header files. This is because we may want to provide a C-callabl
Use all all-lowercase characters for file names. This is consistent with the current recommendation for python, and so it will make the file name convention consistent throughout the BIND10 source tree. Not mixing lower/upper cases will also help avoid name conflicts in a case insensitive file system. Note that this policy may not compatible with C++ class name convention (see below) if the file name is based on the class name (e.g., name "myclass.cc" for the definition of the "Myclass" class). We explicitly accept the conflict, but note that this means it will effectively prohibit mixing cases in class names ("Myclass" and "!MyClass" may not coexist).
Note header files should not include config.h.
</details>
## Ordering Include Files
......@@ -78,32 +91,6 @@ Incorrect:
#include "name.h"
```
## Line length
Source code not exceeding 80 columns is preferred. This is derived from
the [wiki:BIND9CodingGuidelines BIND 9 coding guidelines], mainly for
style consistency. However, C++ names (especially with namespaces) are significantly
longer, thus strictly adhering to this causes overly wrapped code that is both
ugly and difficult to read. Therefore it is permitted to use lines up to 100 columns long.
Rationale: It is 2015 and all ISC employees have equipment that is at most 3 years old. You should have
FullHD (or better) display available to you. You should be able to display two files side by side comfortably.
## Tabs & Indentation
Do not use hard tabs.
Indentation at each level is 4 spaces for C++, other languages should use what is "usual and expected."
In C++ we use the BSD style (also from BIND 9), where continuing lines are aligned with the corresponding opening parenthesis, like this:
```cpp
if (JS_DefineProperty(cx, o, "data",
STRING_TO_JSVAL(JS_NewStringCopyN(cx, data, res)),
0, 0, JSPROP_ENUMERATE) != 0) {
```
## Curly Braces
Always add braces even for a single-line block:
......@@ -202,7 +189,6 @@ Use the prefix form of increment/decrement operators by default:
}
```
Preferring the prefix form of these operators is a well known practice
for non trivial types due to performance reasons. And, for
consistency, we use the same style for basic types like `int` even if
......@@ -216,7 +202,7 @@ context, leave a comment about why the different form is used (since
it's subjective whether it's "obvious", it's generally a good idea to
leave the comment in this case).
## Operator Overloading ##
### Operator Overloading
Operator overloading is allowed when it's considered intuitive and
helpful for improving code readability. But care should be taken,
......@@ -230,21 +216,6 @@ It's recommended to define `operator<<(std::ostream& os, const TheClass& obj)`
if `TheClass` has `operator==()` and `toText()` methods. This allows
the class can be used in `EXPECT_EQ` (and its variants) in googletests.
The following rule was deprecated. It doesn't seem to be followed
anyway, and no one remembered why it had been introduced.
~~When a class supports operator overloading, then there should also
be non-overloaded methods:~~
```cpp
class Foo {
public:
// This rule was deprecated.
//bool equals(const Foo& other) const;
bool operator==(const Foo& other) const { return (equals(other)); }
};
```
## Explicit Constructors
By default C++ constructors with one argument are conversion functions. When they are not (which is the general case) they should be explicit as in:
......@@ -258,9 +229,9 @@ public:
## Class Attributes
Accessors for class attributes should be called '''`getXxx()`'''.
Accessors for class attributes should be called `getXxx()`.
Mutators for class attributes should be called '''`setXxx()`'''.
Mutators for class attributes should be called `setXxx()`.
(where xxx is the attribute)
......@@ -268,12 +239,13 @@ Mutators for class attributes should be called '''`setXxx()`'''.
If you want a class to be non-copyable (neither copy constructor nor assignment), inherit from boost::noncopyable rather than implementing this yourself.
http://www.boost.org/doc/libs/1_47_0/libs/utility/utility.htm#Class_noncopyable
http://www.boost.org/doc/libs/1_77_0/libs/utility/utility.htm#Class_noncopyable
## Naming
Don't start things with underscores. According to Stroustrup's C++ book:
Names starting with an underscore are reserved for special facilities in the implementation and the run-time environment, so such names should not be used in application programs.
> Names starting with an underscore are reserved for special facilities in the implementation and the run-time environment, so such names should not be used in application programs.
Class names are '''`LikeThis`''', methods are '''`likeThis()`''', variables are '''`like_this`''', and constants are '''`LIKE_THIS`'''. Data class members are '''`like_this_`'''.
......@@ -293,7 +265,7 @@ enum ExamplePortNumbers {
};
```
== Where to Put Reference and Pointer Operators ==
## Where to Put Reference and Pointer Operators
In C++, it seems to be more common to not insert a space between the
type and the operator:
......@@ -343,7 +315,7 @@ When writing a Doxygen special comment block there are several possible styles:
http://www.stack.nl/~dimitri/doxygen/docblocks.html
Doxygen keywords should be prepended with @, not with a backslash. The reason to prefer @ is that backslash may confuse scripts that would go over the code. There is some inconsistency in this regard. There are large parts of older code that still use backslash. It is a matter of personal taste to keep consistency with what is in the file vs. strictly sticking with this principle.
Doxygen keywords should be prepended with @, not with a backslash. The reason to prefer @ is that backslash may confuse scripts that would go over the code. There is some inconsistency in this regard. There are large parts of older code that still use backslash. It is a matter of personal taste to keep consistency with what is in the file vs. strictly sticking with this principle. Long term we want to use @ everywhere.
We use the C++ style of 3-slashes:
......@@ -377,12 +349,16 @@ class Good {
void good2();
};
```
### Explicit @brief for Doxygen
If you don't use @brief as the first thing in your doxygen comment, then doxygen will turn the first paragraph into a @brief description anyway. However, we include it anyway so that everybody understands that this is the @brief description.
### TODO Comments
We sprinkle comments in code with keywords to indicate pending work.
### Explicit @brief for Doxygen
In Kea, `///@todo` is preferred. It should be prepended with triple ///, so it will show up on a nicely auto-generated Doxygen todo list. If there is a corresponding ticket, feel free to specify its number in the comment. Unless other wise specified, issue #1234 means a ticket in the gitlab, available at http://gitlab.isc.org/isc-projects/kea
If you don't use @brief as the first thing in your doxygen comment, then doxygen will turn the first paragraph into a @brief description anyway. However, we include it anyway so that everybody understands that this is the @brief description.
## Methods and Functions
......@@ -447,7 +423,7 @@ compiler doesn't help in enforcing it. There's a false sense of safety around
it. And it prevents the developer from declaring the method in the derived class
as non-virtual if the design requires it.
### Const references
## Const references
With anything but primitive types (like int or bare pointer), it is better to pass them as const reference when possible, to avoid overhead of calling the copy constructor and copying a lot of data.
......@@ -460,7 +436,7 @@ function(const boost::shared_ptr<DataType>& param) {
}
```
### Exception-safe getters and string-production methods
## Exception-safe getters and string-production methods
Unless there's a compelling reason to do so neither member value getter methods nor and string-production methods, such as toString(), should throw exceptions. Normally a class member is prevented from ever having an invalid value so there is arguably a getter never has a reason to throw, and string-production methods should always be safe to invoke once a class has been instantiated. Both types of methods are commonly used as log statement arguments where one should not have to worry about catching exceptions.
......@@ -507,8 +483,7 @@ Use the "implicit" form when testing to see whether a pointer is 0
}
```
This is different from what is suggested in the [wiki:BIND9CodingGuidelines BIND 9 coding guidelines].
We changed the guideline for BIND 10 for the following reasons:
This is different from what is suggested in the [wiki:BIND9CodingGuidelines BIND 9 coding guidelines]. The guideline was changed for BIND 10 and then adopted in Kea for the following reasons:
- The implicit form is more compatible with smart pointers. For
example, if we need to write a "non-null check" for a
......@@ -547,8 +522,6 @@ for other reasons (bug fix, enhancement, etc). But during the
transition period (which will be long) we'll see both forms.
See also a mailing list discussion starting from: https://lists.isc.org/pipermail/bind10-dev/2012-December/004129.html
and related discussion in a conference call http://bind10.isc.org/wiki/WeeklyMinutes20130115
when we decided to make the change.
This guide line was written before nullptr was added to C++. There was a short discussion
about switching to nullptr at that time and the conclusion was to not do that. So
......@@ -661,9 +634,9 @@ by libprocess and does not include its static definition.
Kea is a server process, so does not have much that would be considered a user interface. This section discusses what we do have. See [Stork coding guidelines](https://gitlab.isc.org/isc-projects/stork/-/wikis/Processes/coding-guidelines) for UI guidelines for Stork.
## IP address and port formatting
# IP address and port formatting
Whenever an IP address and port is output, IETF RFC 2396 and RFC 2732 should be followed.
Whenever an IP address and port is output, IETF [RFC2396](https://datatracker.ietf.org/doc/html/rfc2396) and [RFC2732](https://datatracker.ietf.org/doc/html/rfc2732) should be followed.
For IPv4 addresses, this looks like this:
......@@ -677,7 +650,7 @@ For IPv6 addresses, this looks like this:
If you import code from another project, try to continue the style of the imported project if changes need to be made. This is for two reasons, one is to make merging future versions easier. The other is the encouragement of submitting changes upstream.
# Deprecating options
# Deprecating features
Sometimes we need to introduce changes that are not backward compatible. If possible we try to support the old way while making the new way possible. When introducing new alternative ("new way") that eventually going to replace an existing option ("old way"), there should be at least one stable series which supports both old and new way. If old syntax is used, a warning should be printed urging people to update their syntax.
......@@ -695,7 +668,7 @@ When a schema is changed, we always bump major version and minor always remains
Other projects have their own coding guidelines. Here are some
examples of such guidelines. These are reference purposes only;
unless explicitly stated we also adopt some part of other guidelines,
they are not part of the BIND 10's coding guidelines.
they are not part of the Kea's coding guidelines.
* Google: https://google.github.io/styleguide/cppguide.html
* Mozilla: https://firefox-source-docs.mozilla.org/code-quality/coding-style/index.html
......