Skip to content

Code review

Jump to bottom
Jean-Noël Grad edited this page Jul 15, 2021 · 6 revisions

This page explains how code review is carried out in this repository.

Table of Contents

Automated code review

Automated checks start when a PR is opened. The same checks will be triggered for every push to the branch associated to the PR.

GitLab-CI

The PR is pulled to a GitLab mirror and tested with GitLab-CI. There are three main stages:

  • style stage: checks that the code is correctly formatted and that the Doxygen documentation compiles
  • build stage: compile ESPResSo on various Linux operating systems and run the testsuite
  • additional checks stage: build and check the Sphinx documentation, test the binaries built in the previous stage without a GPU and with a different number of MPI ranks
If the first stage fails, the subsequent stages are skipped and a message is posted to the PR by a bot. This post informs the contributor that the code doesn't conform to our specifications and provides suggestions on how to improve the code. It is common for new contributors to struggle with the bot, usually because they forgot to run pre-commit or didn't install the code style tools.

GitHub Actions

Builds ESPResSo and runs the testsuite on a macOS runner.

codecov

Code coverage generated by the runners in the GitLab-CI pipeline are sent to codecov to generate a report. It can take time for the runners to execute the testsuite, therefore the percentages shown in the first 30 min can be ignored, as they are based on incomplete data.

The codecov/patch — 96% of diff hit (target 94%) line is the most important one. When adding a new feature to ESPResSo, the corresponding code must be fully covered by tests.

kodiak

This is the merge bot. Its status indicates what is missing for the PR to be merged.

Human code review

Once the automated checks have passed, members of the ESPResSo team will start reviewing the code. Only a review by a core team member will count towards mergeability. Sometimes, review can be delegated to a non-core team member, in which case the review will appear as a check mark in a a grey circle until a core team member confirms the review.

Style guide

Reviewers will assess the code and make suggestions for improvement. Below is a non-exhaustive list of common style recommendations.

C++:

  • no use of malloc/free, instead use STL containers or new/delete
  • no use of const_cast, except when interfacing with C libraries
  • no use of C-style casts (unsafe: risk of reinterpret_cast or const_cast), instead use static_cast<T>()
  • everything that can be const should be
  • prefer "East const" in new code
  • avoid bare owning pointers in favor of smart pointers
  • avoid bare loops in favor of STL algorithms and Boost Range algorithms
  • avoid MPI primitives in favor of boost::mpi collectives
  • avoid macros
  • don't introduce new global variables; existing ones are being progressively removed
  • avoid using existing global variables, instead pass them by reference
  • avoid mixing signed and unsigned types, e.g. passing an int to a function that takes a std::size_t
  • CamelCase vs. snake_case: see the dedicated page on Naming conventions and folder structure
  • see also the C++ Core Guidelines; although we do not enforce them, they are an excellent source of good coding practices with detailed explanations
Python and Cython:
  • no wildcard imports, e.g. from espressomd.lb import *
  • avoid importing without a namespace, e.g. from espressomd import lb (prefer import espressomd.lb)
  • no use of eval()
Clone this wiki locally