Maintaining C++ Code Quality With Clang-Tidy

The Alliance isn't just about football. As a tech startup, we're serious about quality software. Some of our performance critical systems, including broadcast streaming, transcoding and archiving, are written in C++17.

As our codebase grows, it becomes increasingly important to enforce programming practices that lead to safe, fast, reliable and maintainable code. A number of well-known guidelines for modern C++ exist:

Additionally, we want to be able to catch as many logical errors as possible before runtime using static analysis. clang-tidy has all of these checks built in so adding it to our build pipeline enables us to catch mistakes before they become bugfix issues.

The following is an example with our current configuration:

#!/usr/bin/env bash

ENABLE_CHECKS=\
cppcoreguidelines-*,\
clang-analyzer-*,\
performance-*,\
readability-*,\
portability-*,\
modernize-*,\
bugprone-*,\
hicpp-*,\
cert-*,\
misc-*,\

DISABLE_CORE_CHECKS=\
-cppcoreguidelines-pro-bounds-array-to-pointer-decay,\
-cppcoreguidelines-pro-bounds-constant-array-index,\
-cppcoreguidelines-pro-bounds-pointer-arithmetic,\
-cppcoreguidelines-pro-type-reinterpret-cast,\
-cppcoreguidelines-pro-type-union-access,\
-cppcoreguidelines-owning-memory,\

DISABLE_HI_CHECKS=\
-hicpp-signed-bitwise,\
-hicpp-no-array-decay,\

DISABLE_MISC_CHECKS=\
-readability-implicit-bool-conversion,\
-modernize-redundant-void-arg,\

echo 'Running clang-tidy...'

find . -name '*.cpp' | xargs clang-tidy -p compile_commands.json \
checks=${ENABLE_CHECKS}${DISABLE_CORE_CHECKS}${DISABLE_HI_CHECKS}${DISABLE_MISC_CHECKS}

echo 'Finished clang-tidy'
Cool. Now let's see it in action
warning: catch handler catches by value; should catch by reference instead [cert-err09-cpp]
} catch (args::Help) {
         ^
         
warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto storage : segmentStorage) {
         ~~~~ ^
         const &
         
warning: redundant get() call on smart pointer [readability-redundant-smartptr-get]
    if (videoConfig.get() == nullptr) {
        ^~~~~~~~~~~~~~~~~
        videoConfig
        
warning: function 'IngestServer::Stream::Stream' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]
        Stream(Logger logger, const Configuration& configuratiton, std::string connectionId);
        ^                                          ~~~~~~~~~~~~~~~
                                                   configuration
                                                   
warning: redundant string initialization [readability-redundant-string-init]
    std::string tag = "";
                      ^~~
                      tag
                      
warning: 'find' called with a string literal consisting of a single character; consider using the more effective overload accepting a character [performance-faster-string-find]
    auto colon = uri.find(":");
                          ^~~~
                          ':'

warning: the parameter 'path' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
AsyncFile::AsyncFile(FileStorage* storage, std::string path) {
                                           ~~~         ^
                                           const      &
                                           
warning: the parameter 'write' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
void Archiver::_write(ArchiveDataType type, std::function<void(uint8_t*)> write, size_t len) {
                                            ~~~                           ^
                                            const                        &
                                            
warning: uninitialized record type: 'tm' [cppcoreguidelines-pro-type-member-init]
        struct tm tm;
        ^           ~
                    {}

warning: do not use 'else' after 'break' [readability-else-after-return]
} else if (err < 0) {
  ^~~~~~~~
          
warning: do not use 'else' after 'return' [readability-else-after-return]
} else if (AVMATCH(&method, &av_createStream)) {
  ^~~~~~~~
          
warning: initialization of '_rtmpLogger' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]
} _rtmpLogger;
  ^

Because we do some low-level bit manipulation in our processing, some of these guidelines are too strict. Luckily, it's just as easy to disable them globaly (see script above), or for a particular line:

// NOLINT(cppcoreguidelines-pro-type-const-cast,misc-macro-parentheses)
Maintaining C++ Code Quality With Clang-Tidy
Share this