... | @@ -24,7 +24,7 @@ Use 192.0.2.0/24 (see [RFC5737](https://tools.ietf.org/html/rfc5737) and 2001:db |
... | @@ -24,7 +24,7 @@ Use 192.0.2.0/24 (see [RFC5737](https://tools.ietf.org/html/rfc5737) and 2001:db |
|
|
|
|
|
We sprinkle comments in code with keywords to indicate pending work.
|
|
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.
|
|
In Stork, @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 Gilab, available at http://gitlab.isc.org/isc-projects/stork.
|
|
</details>
|
|
</details>
|
|
|
|
|
|
## Dead code
|
|
## Dead code
|
... | @@ -35,7 +35,7 @@ Any dead code (both files that are unused and blocks of commented-out code) shou |
... | @@ -35,7 +35,7 @@ Any dead code (both files that are unused and blocks of commented-out code) shou |
|
|
|
|
|
# Go Style
|
|
# Go Style
|
|
|
|
|
|
Used by backend server and agent. Details TBD
|
|
Used by backend server and agent. We use the syntax recommended by `go fmt` command. Please make sure your code adheres before you submit it for review.
|
|
|
|
|
|
# TypeScript Style
|
|
# TypeScript Style
|
|
|
|
|
... | @@ -59,202 +59,12 @@ Do not use hard tabs. |
... | @@ -59,202 +59,12 @@ Do not use hard tabs. |
|
|
|
|
|
Indentation at each level is 4 spaces for C++, other languages should use what is "usual and expected."
|
|
Indentation at each level is 4 spaces for C++, other languages should use what is "usual and expected."
|
|
|
|
|
|
## 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)
|
|
|
|
|
|
|
|
## Naming
|
|
## Naming
|
|
|
|
|
|
Don't start things with underscores. According to Stroustrup's C++ book:
|
|
Don't start things with underscores.
|
|
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_`'''.
|
|
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
|
|
## Comments
|
|
|
|
|
|
Multiline comments can be written in C++ or C style (that is, with // or /* */ marks).
|
|
Multiline comments can be written in C++ or C style (that is, with // or /* */ marks).
|
... | @@ -332,66 +142,6 @@ If you don't use @brief as the first thing in your doxygen comment, then doxygen |
... | @@ -332,66 +142,6 @@ If you don't use @brief as the first thing in your doxygen comment, then doxygen |
|
|
|
|
|
## Methods and Functions
|
|
## 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
|
|
### 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.
|
|
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.
|
... | | ... | |