Skip to content

GitLab

Kea
Kea

coding guidelines · Changes

Page history
Coding guidelines moved to Processes/ authored by Tomek Mrugalski's avatar Tomek Mrugalski
Hide whitespace changes
Inline Side-by-side
Showing with 0 additions and 0 deletions
Processes/coding-guidelines.md 0 → 100644
View page @ ae28edc8
This page documents some coding guidelines.
Refer also to the [wiki:BIND9CodingGuidelines BIND 9 coding guidelines]. They should be used where they do not conflict with the guidelines on this page. This is because we expect many ISC developers work on both versions of code, and in that case it's easier to maintain the code if the styles are as compatible as possible.
Some of the styles derived from BIND 9 that are often forgotten or
misunderstood are explicitly mentioned below.
# Common
<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. 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.
</details>
<details>
<summary> TODO Comments </summary>
We sprinkle comments in code with keywords to indicate pending work.
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>
## Dead code
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.
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.
# Python Style
We don't use python code anymore. If you found any leftovers, feel free to remove them.
# C++ Style
<details>
<summary> File Name Conventions </summary>
Use .cc for C++ source files. This is basically a mere preference and to ensure consistency throughout the package.
Use .h for C++ header files. This is because we may want to provide a C-callable wrapper for some APIs, and some C++ header files are to be included in a C source file. In that case C-compatible file names will look more natural.
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" classs). 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
We include our own project headers first, then library, and finally system headers, whenever possible. Each header is expected to have any necessary #include statements it needs, and this helps insure that.
```cpp
#include <dns/message.h>
#include <boost/shared_ptr.hpp>
#include <string>
```
There can be several exceptions. Some specific libraries require a
particular header file must be included first. A notable example is
Python.h. The top level config.h generated by autoconf should also be
included before other generic header files.
## Include Style
We use the pointy brackets version of `#include`. Also, we use a full path to
the include (relative to some `-I` switch, not to the current directory).
Correct:
```cpp
#include <util/threads/thread.h>
#include <dns/name.h>
```
Incorrect:
```cpp
#include <thread.h>
#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)),
NULL, NULL, JSPROP_ENUMERATE) != 0) {
```
## Curly Braces
Always add braces even for a single-line block:
```cpp
if (something_holds) {
perform something;
} else if (nonorthogonal_condition) {
perform otherthing;
} else { // optionally comment to clarify the fully orthogonal case
perform finalthing;
}
```
### Opening Curly Braces for Functions
The opening curly brace should occur on the same line as the argument list, unless the argument list is more than one line long.
```cpp
void
f(int i) {
// whatever
}
int
g(int i, /* other args here */
int last_argument)
{
return (i * i);
}
```
This was derived from the BIND 9 coding guideline. It's known this style may look awkward (and even may look inconsistent) for some, but for the reason stated at the beginning we follow this style.
### Curly Braces for Catch
A catch statement should have braces on a single line, like this:
```cpp
.
.
.
} catch (const SomeException& ex) {
.
.
.
```
Note if the ex parameter is not used it should be omitted.
## Parentheses
Do put a space after 'return', and also parenthesize the return value.
```cpp
return 1; // BAD
return (1); // Good
```
This was derived from the BIND 9 coding guideline.
## Operators
Use operator methods in a readable way. In particular, use the
following style with `operator==`:
```cpp
if (x == 10) { // Good
// do something that has to be done when x is equal to 10
}
```
instead of this:
```cpp
if (10 == x) { // BAD
// do something that has to be done when x is equal to 10
}
```
because the former style is much more readable and intuitive for
humans. While the latter style might help detect bugs like dropping
one `=` in the expression, modern compilers with proper warning levels
can do the same job more comprehensively. This is especially so for
cleanly written C++ code (compared to plain old C).
See also developers' discussions at:
https://lists.isc.org/pipermail/bind10-dev/2012-March/003266.html
### Increment and Decrement operators (++/--)
Use the prefix form of increment/decrement operators by default:
```cpp
// for (int i = 0; i < 10; i++) { // No good
for (int i = 0; i < 10; ++i) { // Good
// do something for i = 0..10
}
```
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
it's mostly a preference matter for such types. By being consistent,
it will be easier to notice when we use the less efficient style when
it really matters.
Sometimes the context requires the use of postfix form, in which case
it's okay to use that form. But if the intent is not obvious from the
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 is allowed when it's considered intuitive and
helpful for improving code readability. But care should be taken,
because often it could be only intuitive for that developer who
introduced it. If it doesn't look intuitive for the reviewer, the
developer has responsibility to convince the reviewer; if it fails the
default action is to use non operator method/function for that
purpose.
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:
```cpp
class Foo {
public:
// Constructor with one argument
explicit Foo(std::string name);
}
```
## Class Attributes
Accessors for class attributes should be called '''`getXxx()`'''.
Mutators for class attributes should be called '''`setXxx()`'''.
(where xxx is the attribute)
## Non-copyable Classes
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
## 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.
Class names are '''`LikeThis`''', methods are '''`likeThis()`''', variables are '''`like_this`''', and constants are '''`LIKE_THIS`'''. Data class members are '''`like_this_`'''.
Enumerations are written as
```cpp
enum EnumName {
FOO,
BAR
} enum_instance;
```
Note that unless you have a specific reason to set specific values, leave specific values off. These can be written if needed:
```cpp
enum ExamplePortNumbers {
DNS = 53,
DHCP = 68
};
```
== 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:
```cpp
int* int_var;
int& int_ref;
```
## Sizeof Bool
The C++ standard does not require that `sizeof(bool)` is one: it is compiler dependent!
So it should not be used and `sizeof(uint8_t)` is recommended instead.
## Comments
Multiline comments can be written in C++ or C style (that is, with // or /* */ marks).
```cpp
/*
* This is a comment. It is important probably.
*/
```
```cpp
//
// This is a comment. It is important probably.
//
```
```cpp
/* This is also ok. */
// As is this.
```
Comments at the end of lines should usually be C++ style:
```cpp
class Foo {
int bar_length; // The length of the bar in millimeters.
};
```
### Doxygen Comment Style
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.
We use the C++ style of 3-slashes:
```cpp
/// A lot of examples are called foo().
///
/// @param baz foo() usually takes an argument
void
foo(Bar baz) {
...
}
```
Make sure inserting a blank line between two function/method
declarations or definitions:
```cpp
class Bad {
/// @brief Short description for bad1
void bad1();
/// @brief Short description for bad2, which may also look for bad1().
void bad2();
};
```
```cpp
class Good {
/// @brief Short description for good1
void good1();
/// @brief Short description for good2, which should be much clearer.
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.
## Methods and Functions
### Opening Curly Braces
For methods where the arguments all fit on one line with the curly brace, it should be written on one line:
```cpp
int
methodName(int argument_one, std::string message) {
...
}
```
Where this is not possible the curly brace should go on a line by itself:
```cpp
int
methodName(int argument_one, std::string message,
int another_argument)
{
...
}
```
### Virtual Methods
Explicitly add `virtual` to method declarations in derived classes:
```cpp
class Base {
// this 'virtual' is absolutely necessary
virtual void toBeVirtual();
};
class Derived : public Base {
// this 'virtual' is not necessarily needed, but add it per this guideline
virtual void toBeVirtual();
}
```
This way it's easier to recognize `Derived::toBeVirtual()` is (more
likely to be) defined as a virtual method in the `Base` class without
referring to the base class definition. It could also mean that
`toBeVirtual` is not defined in the base class and is intended to work
as a virtual methods for classes derived further from `Derived`, but
in practice that's a very rare case; in most cases we use these classes
through the (top) base class interfaces.
### 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.
This includes smart pointers, some of them can be relatively expensive to copy.
```cpp
void
function(const boost::shared_ptr<DataType>& param) {
...
}
```
### 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.
## Namespaces
Namespaces will be lower case: '''isc::dns''', or '''isc::cc'''.
'''using namespace''' should never be used in a header file. They may be used in .cc source files.
## Consts
Use `const` as much as possible. Make class methods const member functions whenever possible.
If a function has a parameter with a "top-level const", make sure the `const` appears not only in the definition but also in the declaration. For example, if you define the following function in a .cc file:
```cpp
int
foo(const int param) {
...
}
```
then make sure the `const` appears in the corresponding declaration in a header file (.h):
```cpp
int foo(const int param); // const cannot be omitted
```
Technically, the latter `const` is redundant. But !SunStudio C++ compilers have a bug in name mangling that requires the "consistency": http://developers.sun.com/sunstudio/documentation/ss12/mr/READMEs/c++.html#cNameMangling
Unfortunately, we want to be as portable as possible, and so need to work around this by being redundant.
## NULL pointer check
Use the "implicit" form when testing to see whether a pointer is 0
(`NULL`):
```cpp
SomeType* ptr;
if (ptr) { // instead of 'ptr != NULL'
// do something if the value of ptr is not 0
}
if (!ptr) { // instead of 'ptr == NULL'
// do something if the value of ptr 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:
- The implicit form is more compatible with smart pointers. For
example, if we need to write a "non-null check" for a
`boost::shared_ptr` value without its type conversion (to `bool`)
operator, it would look quite ugly:
```cpp
boost::shared_ptr<SomeType> ptr;
if (ptr.get() != NULL) { // ugly
// do something if the underlying pointer value of ptr is not 0
}
if (ptr) { // same effect, more concise
// ditto
}
```
So for smart pointers we definitely want to use the latter, and it's
better to use the consistent form for both smart pointers and bare
pointers. (And, in any case, it's generally better to minimize
direct use of bare pointers).
- For some developers the "implicit" form was more intuitive, while
others had no strong preference.
- This has been one of the most-broken guidelines (perhaps partly
because of the incompatibility with the smart pointer usage). If a
guideline is so difficult to meet, it probably means we should
revisit the guideline rather than pursuing how to enforce it.
- The implicit form doesn't weaken code safety.
- `NULL` is usually defined as a macro. In general, it's better to
avoid macros in C++ code.
Note that there are still many NULL checks in the "explicit" form in
the source tree as of this writing (April 2013) as we began with the
BIND 9 style. New code should follow this guideline, and it's
encouraged to update existing code gradually while we update the code
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.
## Log Statement Safety
It is extremely important to examine all arguments passed into a log
statement to ensure they will produce safe values at runtime:
- Can the argument (or any part of it) be NULL? If so is this taken into account?
- If the argument invokes any fuctions, are they exception safe?
- If it involves indirection, does this always resolve into a usable value?
- If it raises an exception, is the exception caught? This includes double errors, i.e., log statements in an exception handler.
Log statements are less than helpful if they cause the program to segfault or throw.
## Google Test Style
Try to use EXPECT_<op>() rather than EXPECT_TRUE():
```cpp
EXPECT_TRUE(run_time > 1); // BAD
EXPECT_GT(run_time, 1); // Good
```
because when the test fails the latter will provide more detailed
information (the values of "actual" and "expected").
Note that this is not always possible, especially when checking an
object that has no operator<<() (or more obviously no operator<op>()),
which is used to output on failure.
## Test naming
Names of tests should be similar to classes and method names for that language. For example:
* for gtest C++ tests, you'd use `CamelCaseTest` as the fixture class name, and `testName()` as the test name (as it is a C++ method).
* for Python unittests, you'd use `CamelCaseTest` as the fixture class name, and `test_something()` as the test name (as it is a Python method).
# Makefile.am Guidelines
## Subdirectories
If there is are one or more subdirectories, the first line in the file
should be the SUBDIRS entry, e.g.:
```
SUBDIRS = . tests
```
If relevant (as is usually the case), the current directory should be
included in the list at the appropriate point in the list.
## Preprocessor flags / includes
Next should be the setting of the AM_CPPFLAGS, which applies to every
module built with C++ in the Makefile.am, e.g.:
```
AM_CPPFLAGS = -I$(top_srcdir)/src/lib -I$(top_builddir)/src/lib
AM_CPPFLAGS += $(BOOST_INCLUDES)
```
This can be overriden on a per-module basis in the file by specifying
the variable module_CPPFLAGS. (Note that if the CPPFLAGS environment
variable is set, its flags are appended to the command line after
AM_CPPFLAGS.)
Directory variables should be relative (i.e., no abs_top_builddir) for efficiency.
Third (or more) levels should not be used, e.g.:
```
NO += -I$(top_srcdir)/src/lib/dns -I$(top_builddir)/src/lib/dns
```
AM_CXXFLAGS should be initialized to KEA_CXXFLAGS, e.g.:
```
AM_CXXFLAGS = $(KEA_CXXFLAGS)
```
## Linker flags
When executables are build, AM_LDFLAGS must be conditionally set to static, e.g.:
```
if USE_STATIC_LINK
AM_LDFLAGS = -static
endif
```
## Library dependencies
With the exception of archives, (dynamic) libraries and executables should be linked
with all dependencies in the opposite order of src/lib/Makefile.am for
shared libraries (so finishing by libkea-exceptions.la) followed by external dependencies.
The idea is to not rely on libtool which has shown some problems handling dependencies.
Dynamic Shared Objects built as modules to be dynamically loaded must have
no dependency when static linking is enforced: their undefined symbols
should be resolved using executables they are loaded into.
# User Interface (UI) Guidelines
BIND 10 is a server process, so does not have much that would be considered a user interface. This section discusses what we do have.
## IP address and port formatting
Whenever an IP address and port is output, IETF RFC 2396 and RFC 2732 should be followed.
For IPv4 addresses, this looks like this:
192.0.2.1:53
For IPv6 addresses, this looks like this:
[2001:db8::1]:53
# Imported Code
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.
# Guidelines Adopted by Other Projects
Other projects have their own coding guidelines. Here're 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.
* Google: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml
* Mozilla: https://developer.mozilla.org/en/Mozilla_Coding_Style_Guide
* XORP: http://cvsweb.xorp.org/cgi-bin/cvsweb.cgi/xorp/devnotes/coding-style.txt?rev=1.7;content-type=text%2Fplain