Update Code Review Guidelines with Exceptions section
Reviewers: mtomic, mferencevic, msantl, ipaljak, vkasljevic, dlozic, buda Reviewed By: mferencevic, msantl, buda Differential Revision: https://phabricator.memgraph.io/D2286
This commit is contained in:
parent
fb6dc83ef4
commit
a3d99d7628
@ -3,6 +3,34 @@
|
||||
This chapter describes some of the things you should be on the lookout when
|
||||
reviewing someone else's code.
|
||||
|
||||
## Exceptions
|
||||
|
||||
Although the Google C++ Style Guide forbids exceptions, we do allow them in
|
||||
our codebase. As a reviewer you should watch out for the following.
|
||||
|
||||
The documentation of throwing functions needs to be in-sync with the
|
||||
implementation. This must be enforced recursively. I.e. if a function A now
|
||||
throws a new exception, and the function B uses A, then B needs to handle that
|
||||
exception or have its documentation updated and so on. Naturally, the same
|
||||
applies when an exception is removed.
|
||||
|
||||
Transitive callers of the function which throws a new exception must be OK
|
||||
with that. This ties into the previous point. You need to check that all users
|
||||
of the new exception either handle it correctly or propagate it.
|
||||
|
||||
Exceptions should not escape out of class destructors, because that will
|
||||
terminate the program. The code should be changed so that such cases are not
|
||||
possible.
|
||||
|
||||
Exceptions being thrown in class constructors. Although this is well defined
|
||||
in C++, it usually implies that a constructor is doing too much work and the
|
||||
class construction & initialization should be redesigned. Usual approaches are
|
||||
using the (Static) Factory Method pattern or having some sort of an
|
||||
initialization method that needs to be called after the construction is done.
|
||||
Prefer the Factory Method.
|
||||
|
||||
Don't forget that STL functions may also throw!
|
||||
|
||||
## Pointers & References
|
||||
|
||||
In cases when some code passes a pointer or reference, or if a code stores a
|
||||
|
@ -8,6 +8,9 @@ C++ code.
|
||||
Memgraph uses the
|
||||
[Google Style Guide for C++](https://google.github.io/styleguide/cppguide.html)
|
||||
in most of its code. You should follow them whenever writing new code.
|
||||
Besides following the style guide, take a look at
|
||||
[Code Review Guidelines](code-review.md) for common design issues and pitfalls
|
||||
with C++ as well as [Required Reading](required-reading.md).
|
||||
|
||||
### Often Overlooked Style Conventions
|
||||
|
||||
@ -39,8 +42,8 @@ code. Unfortunately, they do have 2 major downsides.
|
||||
|
||||
For those reasons the style guide recommends minimal work that cannot fail.
|
||||
Using virtual methods or doing a lot more should be delegated to some form of
|
||||
`Init` method, possibly coupled with factory functions. Similar rules apply to
|
||||
destructors, which are not allowed to even throw exceptions.
|
||||
`Init` method, possibly coupled with static factory methods. Similar rules
|
||||
apply to destructors, which are not allowed to even throw exceptions.
|
||||
|
||||
[Style guide reference](https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors)
|
||||
|
||||
@ -64,6 +67,10 @@ exception even though it is not the main thread.
|
||||
|
||||
[Style guide reference](https://google.github.io/styleguide/cppguide.html#Exceptions)
|
||||
|
||||
In general, when introducing a new exception, either via `throw` statement or
|
||||
calling a function which throws, you must examine all transitive callers and
|
||||
update their implementation and/or documentation.
|
||||
|
||||
#### Assertions
|
||||
|
||||
We use `CHECK` and `DCHECK` macros from glog library. You are encouraged to
|
||||
|
Loading…
Reference in New Issue
Block a user