-
Notifications
You must be signed in to change notification settings - Fork 0
Test Code Guide line
-
Types start with uppercase:
MyClass. -
Functions and variables start with lowercase:
myMethod. -
Constants are all upper case:
const double PI=3.14159265358979323;. -
Pointers
void* pand notvoid *porvoid * p -
References
int& pand notint &porint & p -
When using
auto, to avoid confusion, writeauto& vorauto* pto avoid ambiguity.
class A {
};
struct B {
};
void doSomething() {
if (condition) {
doThis();
} else {
doThat();
}
while (condition()) {
continue;
}
for (int i = 0; i < a; ++i) {
break;
}
}C++11 introduces nullptr which is a special value denoting a null pointer. This should be used instead of 0 or NULL to indicate a null pointer.
Comment blocks should use //, not /* */. Using // makes it much easier to comment out a block of code while debugging.
/ this function does something
int myFunc(){}To comment out this function block during debugging we might do:
/*
// this function does something
int myFunc(){}*
which would be impossible if the function comment header used /* */.
This causes the namespace you are using to be pulled into the namespace of all files that include the header file.
It pollutes the namespace and it may lead to name collisions in the future.
Writing using namespace in an implementation file is fine though.
Header files must contain a distinctly-named include guard to avoid problems with including the same header multiple times and to prevent conflicts with headers from other projects.
#ifndef MYPROJECT_MYCLASS_HPP
#define MYPROJECT_MYCLASS_HPP
namespace MyProject {
class MyClass {
};
}
#endif
You may also consider using the #pragma once directive instead which is quasi-standard across many compilers.
It's short and makes the intent clear.
... <> is reserved for system includes.
// Bad Idea. Requires extra -I directives to the compiler
// and goes against standards.
#include <string>
#include <includes/MyHeader.hpp>
// Worse Idea
// Requires potentially even more specific -I directives and
// makes code more difficult to package and distribute.
#include <string>
#include <MyHeader.hpp>
// Good Idea
// Requires no extra params and notifies the user that the file
// is a local file.
#include <string>
#include "MyHeader.hpp"
...with the member initializer list.
For POD types, the performance of an initializer list is the same as manual initialization, but for other types there is a clear performance gain, see below.
// Bad Idea
class MyClass {
public:
MyClass(int t_value) {
m_value = t_value;
}
private:
int m_value;
};
// Bad Idea
// This leads to an additional constructor call for m_myOtherClass
// before the assignment.
class MyClass {
public:
MyClass(MyOtherClass t_myOtherClass) {
m_myOtherClass = t_myOtherClass;
}
private:
MyOtherClass m_myOtherClass;
};
// Good Idea
// There is no performance gain here but the code is cleaner.
class MyClass{
public:
MyClass(int t_value): m_value(t_value){}
private:
int m_value;
};
// Good Idea
// The default constructor for m_myOtherClass is never called here, so
// there is a performance gain if MyOtherClass is not is_trivially_default_constructible.
class MyClass {
public:
MyClass(MyOtherClass t_myOtherClass) : m_myOtherClass(t_myOtherClass){}
private:
MyOtherClass m_myOtherClass;
};In C++11 you can assign default values to each member (using = or using {}).
// ... //
private:
int m_value = 0; // allowed
unsigned m_value_2 = -1; // narrowing from signed to unsigned allowed
// ... //
This ensures that no constructor ever "forgets" to initialize a member object.
Using brace initialization does not allow narrowing at compile-time.
// Best Idea
// ... //
private:
int m_value{ 0 }; // allowed
unsigned m_value_2 { -1 }; // narrowing from signed to unsigned not allowed, leads to a compile time error
// ... //
Prefer {} initialization over = unless you have a strong reason not to.
Forgetting to initialize a member is a source of undefined behavior bugs which are often extremely hard to find.
If the member variable is not expected to change after the initialization, then mark it const.
class MyClass
{
public:
MyClass(int t_value)
: m_value{t_value}
{
}
private:
const int m_value{0};
};
The standard library generally uses std::size_t for anything related to size. The size of size_t is implementation defined.
In general, using auto will avoid most of these issues, but not all.
Make sure you stick with the correct integer types and remain consistent with the C++ standard library. It might not warn on the platform you are currently using, but it probably will when you change platforms.
Note that you can cause integer underflow when performing some operations on unsigned values. For example:
std::vector<int> v1{2,3,4,5,6,7,8,9};
std::vector<int> v2{9,8,7,6,5,4,3,2,1};
const auto s1 = v1.size();
const auto s2 = v2.size();
const auto diff = s1 - s2; // diff underflows to a very large number
assert(registerSomeThing()); // make sure that registerSomeThing() returns true
The above code succeeds when making a debug build, but gets removed by the compiler when making a release build, giving you different behavior between debug and release builds.
This is because assert() is a macro which expands to nothing in release mode.
They can help you stick to DRY principles.
They should be preferred to macros, because macros do not honor namespaces, etc.
The Rule of Zero states that you do not provide any of the functions that the compiler can provide (copy constructor, copy assignment operator, move constructor, move assignment operator, destructor) unless the class you are constructing does some novel form of ownership.
The goal is to let the compiler provide optimal versions that are automatically maintained when more member variables are added.
The original article provides the background, while a follow up article explains techniques for implementing nearly 100% of the time.
const tells the compiler that a variable or method is immutable. This helps the compiler optimize the code and helps the developer know if a function has a side effect. Also, using const & prevents the compiler from copying data unnecessarily. The comments on const from John Carmack are also a good read.
// Bad Idea
class MyClass
{
public:
void do_something(int i);
void do_something(std::string str);
};
// Good Idea
class MyClass
{
public:
void do_something(const int i);
void do_something(const std::string &str);
};
-
Getters
-
Returning by
&orconst &can have significant performance savings when the normal use of the returned value is for observation -
Returning by value is better for thread safety and if the normal use of the returned value is to make a copy anyhow, there's no performance lost
-
If your API uses covariant return types, you must return by
&or* -
Temporaries and local values
-
Always return by value.
references: https://github.com/lefticus/cppbestpractices/issues/21 https://twitter.com/lefticus/status/635943577328095232
// Very Bad Idea
class MyClass
{
public:
explicit MyClass(const int& t_int_value)
: m_int_value(t_int_value)
{
}
const int& get_int_value() const
{
return m_int_value;
}
private:
int m_int_value;
}
Instead, pass and return simple types by value. If you plan not to change passed value, declare them as const, but not const refs:
// Good Idea
class MyClass
{
public:
explicit MyClass(const int t_int_value)
: m_int_value(t_int_value)
{
}
int get_int_value() const
{
return m_int_value;
}
private:
int m_int_value;
}
Why? Because passing and returning by reference leads to pointer operations instead by much faster passing values in processor registers.
Raw memory access, allocation and deallocation, are difficult to get correct in C++ without risking memory errors and leaks. C++11 provides tools to avoid these problems.
// Bad Idea
MyClass *myobj = new MyClass;
// ...
delete myobj;
// Good Idea
auto myobj = std::make_unique<MyClass>(constructor_param1, constructor_param2); // C++14
auto myobj = std::unique_ptr<MyClass>(new MyClass(constructor_param1, constructor_param2)); // C++11
auto mybuffer = std::make_unique<char[]>(length); // C++14
auto mybuffer = std::unique_ptr<char[]>(new char[length]); // C++11
// or for reference counted objects
auto myobj = std::make_shared<MyClass>();
// ...
// myobj is automatically freed for you whenever it is no longer used.
Exceptions cannot be ignored. Return values, such as using boost::optional, can be ignored and if not checked can cause crashes or memory errors. An exception, on the other hand, can be caught and handled. Potentially all the way up the highest level of the application with a log and automatic restart of the application.
Stroustrup, the original designer of C++, makes this point much better than I ever could.
Variadic functions can accept a variable number of parameters. The probably best known example is printf(). You have the possibility to define this kind of functions by yourself but this is a possible security risk. The usage of variadic functions is not type safe and the wrong input parameters can cause a program termination with an undefined behavior. This undefined behavior can be exploited to a security problem.
If you have the possibility to use a compiler that supports C++11, you can use variadic templates instead.
Global data leads to unintended side effects between functions and can make code difficult or impossible to parallelize. Even if the code is not intended today for parallelization, there is no reason to make it impossible for the future.
Besides being global data, statics are not always constructed and deconstructed as you would expect. This is particularly true in cross-platform environments. See for example, this g++ bug regarding the order of destruction of shared static data loaded from dynamic modules.
std::shared_ptr is "as good as a global" (http://stackoverflow.com/a/18803611/29975) because it allows multiple pieces of code to interact with the same data.
A singleton is often implemented with a static and/or shared_ptr.
Much slower in threaded environments. In many or maybe even most cases, copying data is faster. Plus with move operations and such and things.
For member variables it is good practice to use mutex and mutable together. This applies in both ways:
-
A mutable member variable is presumed to be a shared variable so it should be synchronized with a mutex (or made atomic)
-
If a member variable is itself a mutex, it should be mutable. This is required to use it inside a const member function.
For more information see the following article from Herb Sutter: http://herbsutter.com/2013/05/24/gotw-6a-const-correctness-part-1-3/
See also related safety discussion about const & return values
This:
// some header file
class MyClass;
void doSomething(const MyClass &);
instead of:
// some header file
#include "MyClass.hpp"
void doSomething(const MyClass &);
This applies to templates as well:
template<typename T> class MyTemplatedType;
This is a proactive approach to reduce compilation time and rebuilding dependencies.
The compiler has to do something with each include directive it sees. Even if it stops as soon as it sees the #ifndef include guard, it still had to open the file and begin processing it.
include-what-you-use is a tool that can help you identify which headers you need.
There's no real way to know where your bottlenecks are without analyzing the code.
The cleaner, simpler, and easier to read the code is, the better chance the compiler has at implementing it well.
// This
std::vector<ModelObject> mos{mo1, mo2};
// -or-
auto mos = std::vector<ModelObject>{mo1, mo2};
// Don't do this
std::vector<ModelObject> mos;
mos.push_back(mo1);
mos.push_back(mo2);
Initializer lists are significantly more efficient; reducing object copies and resizing of containers.
// Instead of
auto mo1 = getSomeModelObject();
auto mo2 = getAnotherModelObject();
doSomething(mo1, mo2);
// consider:
doSomething(getSomeModelObject(), getAnotherModelObject());
This sort of code prevents the compiler from performing a move operation...
Make sure you understand this document: https://mbevin.wordpress.com/2012/11/20/move-semantics/
shared_ptr objects are much more expensive to copy than you'd think they would be. This is because the reference count must be atomic and thread-safe. So this comment just re-enforces the note above: avoid temporaries and too many copies of objects. Just because we are using a pImpl it does not mean our copies are free.
For more simple cases, the ternary operator can be used:
// Bad Idea
std::string somevalue;
if (caseA) {
somevalue = "Value A";
} else {
somevalue = "Value B";
}
// Better Idea
const std::string somevalue = caseA ? "Value A" : "Value B";
More complex cases can be facilitated with an immediately-invoked lambda.
// Bad Idea
std::string somevalue;
if (caseA) {
somevalue = "Value A";
} else if(caseB) {
somevalue = "Value B";
} else {
somevalue = "Value C";
}
// Better Idea
const std::string somevalue = [&](){
if (caseA) {
return "Value A";
} else if (caseB) {
return "Value B";
} else {
return "Value C";
}
}();
Exceptions which are thrown and captured internally during normal processing slow down the application execution. They also destroy the user experience from within a debugger, as debuggers monitor and report on each exception event. It is best to just avoid internal exception processing when possible.
We already know that we should not be using raw memory access, so we are using unique_ptr and shared_ptr instead, right?
Heap allocations are much more expensive than stack allocations, but sometimes we have to use them. To make matters worse, creating a shared_ptr actually requires 2 heap allocations.
However, the make_shared function reduces this down to just one.
std::shared_ptr<ModelObject_Impl>(new ModelObject_Impl());
// should become
std::make_shared<ModelObject_Impl>(); // (it's also more readable and concise)
If possible use unique_ptr instead of shared_ptr. The unique_ptr does not need to keep track of its copies because it is not copyable. Because of this it is more efficient than the shared_ptr. Equivalent to shared_ptr and make_shared you should use make_unique (C++14 or greater) to create the unique_ptr:
std::make_unique<ModelObject_Impl>();
Current best practices suggest returning a unique_ptr from factory functions as well, then converting the unique_ptr to a shared_ptr if necessary.
std::unique_ptr<ModelObject_Impl> factory();
auto shared = std::shared_ptr<ModelObject_Impl>(factory());
std::endl implies a flush operation. It's equivalent to "\n" << std::flush.
Variables should be declared as late as possible, and ideally only when it's possible to initialize the object. Reduced variable scope results in less memory being used, more efficient code in general, and helps the compiler optimize the code further.
// Good Idea
for (int i = 0; i < 15; ++i)
{
MyObject obj(i);
// do something with obj
}
// Bad Idea
MyObject obj; // meaningless object initialization
for (int i = 0; i < 15; ++i)
{
obj = MyObject(i); // unnecessary assignment operation
// do something with obj
}
// obj is still taking up memory for no reason
For C++17 and onwards, consider using init-statement in the if and switch statements:
if (MyObject obj(index); obj.good()) {
// do something if obj is good
} else {
// do something if obj is not good
}
This topic has an associated discussion thread.
Depending on the situation and the compiler's ability to optimize, one may be faster over the other. Choosing float will result in lower precision and may be slower due to conversions. On vectorizable operations float may be faster if you are able to sacrifice precision.
double is the recommended default choice as it is the default type for floating-point values in C++.
See this stackoverflow discussion for some more information.
... when it is semantically correct. Pre-increment is faster than post-increment because it does not require a copy of the object to be made.
// Bad Idea
for (int i = 0; i < 15; i++)
{
std::cout << i << '\n';
}
// Good Idea
for (int i = 0; i < 15; ++i)
{
std::cout << i << '\n';
}Even if many modern compilers will optimize these two loops to the same assembly code, it is still good practice to prefer ++i. There is absolutely no reason not to and you can never be certain that your code will not pass a compiler that does not optimize this.
You should be also aware that the compiler will not be able to optimize this only for integer types and not necessarily for all iterator or other user-defined types.
The bottom line is that it is always easier and recommended to use the pre-increment operator if it is semantically identical to the post-increment operator.
// Bad Idea
std::cout << someThing() << "\n";
// Good Idea
std::cout << someThing() << '\n';
This is very minor, but a "\n" has to be parsed by the compiler as a const char * which has to do a range check for \0 when writing it to the stream (or appending to a string). A '\n' is known to be a single character and avoids many CPU instructions.
If used inefficiently many times it might have an impact on your performance, but more importantly, thinking about these two usage cases gets you thinking more about what the compiler and runtime have to do to execute your code.