Skip to content

Add clang-tidy to implement some C++17 language features

Dave Moxey requested to merge dmoxey/nektar:feature/c++17-namespace into master

Issue/feature addressed

Transitioning to a more modern C++ standard is important to improve interoperability with other libraries and application development.

Proposed solution

This approach was originally implemented within !1427 (closed); however too many changes were introduced there leading to the MR becoming unreasonably large. In this MR we aim to implement some smaller features of C++17, based on the automatic use of clang-tidy: a more feature-complete linter for C++ code than simple formatting tools such as clang-format.

Implementation

clang-tidy has been configured using the root-level .clang-tidy file to enable the following checks/adjustments:

  • readability-braces-around-statements: enforces the use of braces around statements. This corrects a number of files in which we have used e.g. single line if or for statements.
  • modernize-concat-nested-namespaces: namespace concatenation, i.e. use of namespace Nektar::LibUtilities. This is the only C++17 feature enabled in this branch.
  • modernize-raw-string-literal: conversion of strings containing backslash characters to raw C++ strings
  • modernize-use-nullptr: use of nullptr instead of NULL
  • modernize-use-override: enforce the use of override; this also removes the virtual qualifier since in the presence of override or final this is unnecessary.

To run linting on a local machine, one requires the following series of commands.

First, run the CI to configure CMake, install any third-party dependencies, export all compilation commands (which will be read by clang-tidy) but stop short of compiling the code. This ensures that headers are all in place for clang-tidy.

EXPORT_COMPILE_COMMANDS=1 BUILD_TYPE=full bash -x .gitlab-ci/build-and-test.sh

It is important to note that this tool will only look at files that will be compiled, thus why we need to run in full mode. This means:

  • there might be some files that are still not picked up with some rather more specialised configuration options, so we should make sure that the CI is indeed compiling everything!
  • there are some files I found and removed in !1427 (closed) that are not being compiled; however to keep it cleaner here I suggest removing these in a future MR.

Next, run clang-tidy and tell it to apply all changes:

export CL_T_VER=14  # using clang-tidy-14 by default
run-clang-tidy-$CL_T_VER -p build -header-filter='(library|solver|tests)' \
     -j$NUM_CPUS -fix \
     -clang-tidy-binary=clang-tidy-$CL_T_VER \
     -clang-apply-replacements-binary=clang-apply-replacements-$CL_T_VER

In the above:

  • clang-tidy looks for the compile_commands.json within the build directory specified by -p
  • -header-filter tells clang-tidy where to look for headers for our code: otherwise it may try to apply changes to system headers, which isn't very useful
  • -fix tells clang-tidy to apply the fixes it finds.

Finally, formatting will need to be fixed with clang-format-11 as normal. I suggest keeping this, if possible, since in my testing, later version of clang-format introduce some weird formatting bugs with our particular configuration.

Tests

A new CI template .clang-tidy-template has been added which runs the above on the CI. If the resulting repository has any differences as detected with git diff then these are kept as an artifact, so that changes can be very easily applied by downloading the diff and running git apply.

Suggested reviewers

@ccantwel @CFD-Xing

Notes

Please add any other information that could be useful for reviewers.

Checklist

  • Functions and classes, or changes to them, are documented.
  • User guide/documentation is updated.
  • Changelog is updated.
  • Suitable tests added for new functionality.
  • Contributed code is correctly formatted. (See the contributing guidelines).
  • License added to any new files.
  • No extraneous files have been added (e.g. compiler output or test data files).
Edited by Jacques Xing

Merge request reports