Fix all clang-tidy warnings

Since the code was not compilable with clang for some time and
clang-tidy did not work with gcc due to a gcc-only-compiler option,
clang-tidy was disabled and the warnings piled up.
Now clang-tidy is active for compilation with clang again.

.clang-tidy

@@ -1,5 +1,5 @@
 ---
-Checks: '*,-cppcoreguidelines-avoid-magic-numbers,-readability-magic-numbers,-llvmlibc-implementation-in-namespace,-llvmlibc-callee-namespace,-llvmlibc-restrict-system-libc-headers,-llvm-header-guard,-llvm-include-order,-modernize-use-trailing-return-type,-google-readability-todo,-hicpp-signed-bitwise,-readability-identifier-length,-fuchsia-trailing-return,-cppcoreguidelines-pro-bounds-constant-array-index,-cppcoreguidelines-macro-usage,-bugprone-macro-parentheses,-fuchsia-default-arguments-calls,-hicpp-uppercase-literal-suffix,-readability-uppercase-literal-suffix,-llvm-namespace-comment,-altera-unroll-loops,-altera-id-dependent-backward-branch,-altera-struct-pack-align,-cppcoreguidelines-pro-type-union-access,-bugprone-implicit-widening-of-multiplication-result,-concurrency-mt-unsafe,-fuchsia-default-arguments-declarations,-bugprone-easily-swappable-parameters,-readability-suspicious-call-argument,-fuchsia-overloaded-operator,-readability-convert-member-functions-to-static'
+Checks: '*,-cppcoreguidelines-avoid-magic-numbers,-readability-magic-numbers,-llvmlibc-implementation-in-namespace,-llvmlibc-callee-namespace,-llvmlibc-restrict-system-libc-headers,-llvm-header-guard,-llvm-include-order,-modernize-use-trailing-return-type,-google-readability-todo,-hicpp-signed-bitwise,-readability-identifier-length,-fuchsia-trailing-return,-cppcoreguidelines-pro-bounds-constant-array-index,-cppcoreguidelines-macro-usage,-bugprone-macro-parentheses,-fuchsia-default-arguments-calls,-hicpp-uppercase-literal-suffix,-readability-uppercase-literal-suffix,-llvm-namespace-comment,-altera-unroll-loops,-altera-id-dependent-backward-branch,-altera-struct-pack-align,-cppcoreguidelines-pro-type-union-access,-bugprone-implicit-widening-of-multiplication-result,-concurrency-mt-unsafe,-fuchsia-default-arguments-declarations,-bugprone-easily-swappable-parameters,-readability-suspicious-call-argument,-fuchsia-overloaded-operator,-readability-convert-member-functions-to-static,-fuchsia-statically-constructed-objects'
 WarningsAsErrors: '*'
 HeaderFilterRegex: '.*'
 AnalyzeTemporaryDtors: false

CMakeLists.txt

@@ -1,4 +1,4 @@
-cmake_minimum_required(VERSION 3.17)
+cmake_minimum_required(VERSION 3.25)
 project(game_boy_emulator)
 
 set(CMAKE_CXX_STANDARD 20)
@@ -22,6 +22,8 @@ find_package(Boost REQUIRED)
 include(FetchContent)
 
 FetchContent_Declare(nativefiledialog-extended
+        # Without this clang-tidy would warn about things in the libraries header, but this requires cmake 3.25
+        SYSTEM
         GIT_REPOSITORY https://github.com/btzy/nativefiledialog-extended.git
         GIT_TAG v1.0.1
         )
@@ -34,7 +36,16 @@ set(CMAKE_MODULE_PATH ${CMAKE_SOURCE_DIR}/cmake/modules
 set(CLANG_FORMAT_EXCLUDE_PATTERNS "src/bindings" ${CMAKE_BINARY_DIR})
 find_package(ClangFormat)
 
-include(clang-tidy)
+if (CMAKE_CXX_COMPILER_ID STREQUAL Clang)
+    # The CMAKE_CXX_CLANG_TIDY integration passes all compiler arguments to clang-tidy. For gcc we have a required
+    # compiler option to increase constexpr evaluation limit (fconstexpr-ops-limit), which clang does not know (it has
+    # a different option). But this flag leads to cland-diagnostics-error and the workaround would be manually removing
+    # it from the compilation database. Instead activate clang-tidy only for clang.
+    message(STATUS "Checking with clang-tidy was enabled")
+    include(clang-tidy)
+else()
+    message(STATUS "clang-tidy is only active for clang, disabling checks")
+endif ()
 
 add_compile_options(-fsanitize=address)
 add_link_options(-fsanitize=address)

src/game-boy-emulator/apu.cpp

@@ -29,8 +29,11 @@ uint8_t Apu::read_byte(uint16_t address) {
         m_logger->trace("APU read {:04X}", address);
         switch (address) {
         case NR52_ADDRESS:
-            return m_apu_enabled << 7 | m_channel4.is_enabled() << 3 | m_channel3.is_enabled() << 2
-                   | m_channel2.is_enabled() << 1 | m_channel1.is_enabled();
+            return static_cast<int>(m_apu_enabled) << 7
+                   | static_cast<int>(m_channel4.is_enabled()) << 3
+                   | static_cast<int>(m_channel3.is_enabled()) << 2
+                   | static_cast<int>(m_channel2.is_enabled()) << 1
+                   | static_cast<int>(m_channel1.is_enabled());
         case NR51_ADDRESS:
             return m_sound_panning;
         case NR50_ADDRESS:
@@ -158,6 +161,9 @@ void Apu::cycle_elapsed_callback(size_t cycle_count_m) {
             m_channel1.do_envelope_sweep();
             m_channel2.do_envelope_sweep();
             break;
+        default:
+            assert(false && "There must be an error since this should be unreachable");
+            break;
         }
     }
     m_channel1.tick_wave();
@@ -209,43 +215,6 @@ SampleFrame Apu::get_sample() {
         return {};
     }
 
-    struct ChannelSamples {
-        float ch1 = 0;
-        float ch2 = 0;
-        float ch3 = 0;
-        float ch4 = 0;
-    };
-    auto mix = [&](const ChannelSamples& samples) {
-        // Pan audio channel samples depending on NR51
-        SampleFrame out;
-        auto options = m_emulator->get_options();
-        if (options.apu_channel1_enabled && bitmanip::is_bit_set(m_sound_panning, CH1_LEFT)) {
-            out.left += samples.ch1;
-        }
-        if (options.apu_channel1_enabled && bitmanip::is_bit_set(m_sound_panning, CH1_RIGHT)) {
-            out.right += samples.ch1;
-        }
-        if (options.apu_channel2_enabled && bitmanip::is_bit_set(m_sound_panning, CH2_LEFT)) {
-            out.left += samples.ch2;
-        }
-        if (options.apu_channel2_enabled && bitmanip::is_bit_set(m_sound_panning, CH2_RIGHT)) {
-            out.right += samples.ch2;
-        }
-        if (options.apu_channel3_enabled && bitmanip::is_bit_set(m_sound_panning, CH3_LEFT)) {
-            out.left += samples.ch3;
-        }
-        if (options.apu_channel3_enabled && bitmanip::is_bit_set(m_sound_panning, CH3_RIGHT)) {
-            out.right += samples.ch3;
-        }
-        if (options.apu_channel4_enabled && bitmanip::is_bit_set(m_sound_panning, CH4_LEFT)) {
-            out.left += samples.ch4;
-        }
-        if (options.apu_channel4_enabled && bitmanip::is_bit_set(m_sound_panning, CH4_RIGHT)) {
-            out.right += samples.ch4;
-        }
-        return out;
-    };
-
     ChannelSamples samples;
     if (m_channel1.is_enabled()) {
         // Channel output is 0..15, DAC converts it to -1..1
@@ -263,15 +232,46 @@ SampleFrame Apu::get_sample() {
     return mixed_sample;
 }
 
-uint8_t Apu::get_left_output_volume() const {
+float Apu::get_left_output_volume() const {
     // A value of 0 is treated as volume 1 and a value of 7 is treated as volume 8 (no reduction).
-    return ((m_master_volume & 0b01110000) >> 4) + 1;
+    return static_cast<float>(((m_master_volume & 0b01110000) >> 4) + 1);
 }
 
-uint8_t Apu::get_right_output_volume() const {
-    return (m_master_volume & 0b111) + 1;
+float Apu::get_right_output_volume() const {
+    return static_cast<float>((m_master_volume & 0b111) + 1);
 }
 
 float Apu::convert_dac(uint8_t value) {
-    return (value - (15.f / 2.f)) / 7.5f;
+    return (static_cast<float>(value) - (15.f / 2.f)) / 7.5f;
+}
+
+SampleFrame Apu::mix(const ChannelSamples& samples) {
+    // Pan audio channel samples depending on NR51
+    SampleFrame out;
+    auto options = m_emulator->get_options();
+    if (options.apu_channel1_enabled && bitmanip::is_bit_set(m_sound_panning, CH1_LEFT)) {
+        out.left += samples.ch1;
+    }
+    if (options.apu_channel1_enabled && bitmanip::is_bit_set(m_sound_panning, CH1_RIGHT)) {
+        out.right += samples.ch1;
+    }
+    if (options.apu_channel2_enabled && bitmanip::is_bit_set(m_sound_panning, CH2_LEFT)) {
+        out.left += samples.ch2;
+    }
+    if (options.apu_channel2_enabled && bitmanip::is_bit_set(m_sound_panning, CH2_RIGHT)) {
+        out.right += samples.ch2;
+    }
+    if (options.apu_channel3_enabled && bitmanip::is_bit_set(m_sound_panning, CH3_LEFT)) {
+        out.left += samples.ch3;
+    }
+    if (options.apu_channel3_enabled && bitmanip::is_bit_set(m_sound_panning, CH3_RIGHT)) {
+        out.right += samples.ch3;
+    }
+    if (options.apu_channel4_enabled && bitmanip::is_bit_set(m_sound_panning, CH4_LEFT)) {
+        out.left += samples.ch4;
+    }
+    if (options.apu_channel4_enabled && bitmanip::is_bit_set(m_sound_panning, CH4_RIGHT)) {
+        out.right += samples.ch4;
+    }
+    return out;
 }

src/game-boy-emulator/apu.hpp

@@ -40,16 +40,29 @@ class Apu {
     ClockTimer m_frame_sequencer_timer;
 
     // Get right/left volume from NR50
-    uint8_t get_left_output_volume() const;
-    uint8_t get_right_output_volume() const;
+    [[nodiscard]] float get_left_output_volume() const;
+    [[nodiscard]] float get_right_output_volume() const;
+
+    struct ChannelSamples {
+        float ch1 = 0;
+        float ch2 = 0;
+        float ch3 = 0;
+        float ch4 = 0;
+    };
+    /**
+     * Mix all channels into left/right channel according to sound panning register.
+     */
+    SampleFrame mix(const ChannelSamples& samples);
+
+
 
     // Analog-Digital conversion of value in range 0..15 to value in range -1..1
     float convert_dac(uint8_t value);
 
     Emulator* m_emulator;
 
 public:
-    Apu(Emulator* emulator);
+    explicit Apu(Emulator* emulator);
 
     uint8_t read_byte(uint16_t address);
 

src/game-boy-emulator/audio.cpp

@@ -26,20 +26,10 @@ Audio::Audio(Emulator& emulator) :
     audio_spec.format = AUDIO_F32;
     audio_spec.samples = BUFFER_SIZE;
     audio_spec.channels = 2;
-    SDL_AudioSpec obtained{};
     spdlog::info("Using audio device {}", device_ids[0]);
-    m_device_id = SDL_OpenAudioDevice(device_ids[0], SDL_AUDIO_PLAYBACK, &audio_spec, &obtained, 0);
-    assert(obtained.freq == audio_spec.freq && "Audio device configuration mismatch");
-    if (m_device_id == 0) {
-        spdlog::error("Failed to open audio: {}", SDL_GetError());
-        std::exit(EXIT_FAILURE);
-    }
+    m_audio_ressource = AudioRessource(device_ids[0], audio_spec);
     // Start playback on device
-    SDL_PauseAudioDevice(m_device_id, 0);
-}
-
-Audio::~Audio() {
-    SDL_CloseAudioDevice(m_device_id);
+    SDL_PauseAudioDevice(m_audio_ressource.get(), 0);
 }
 
 namespace {
@@ -48,19 +38,42 @@ namespace {
 float calc_volume_log(float volume) {
     return (std::exp(volume) - 1.f) / (std::exp(1.f) - 1.f);
 }
-}
+} // namespace
 
 void Audio::callback(SampleFrame sample) {
     m_resampler.submit_sample_data(sample);
     // Only submit to the actual audio playback if a good amount of samples are available since
     // queuing data involves locks on SDLs side.
     if (m_resampler.available_samples() >= BUFFER_SIZE) {
         auto resampled_data = m_resampler.get_resampled_data();
-        auto volume = calc_volume_log(m_emulator.get_options().volume) * constants::FIXED_VOLUME_SCALE;
+        auto volume
+            = calc_volume_log(m_emulator.get_options().volume) * constants::FIXED_VOLUME_SCALE;
         std::for_each(resampled_data.begin(), resampled_data.end(), [volume](SampleFrame& sf) {
             sf.left *= volume;
             sf.right *= volume;
         });
-        SDL_QueueAudio(m_device_id, resampled_data.data(), static_cast<Uint32>(get_buffersize_bytes(resampled_data)));
+        SDL_QueueAudio(m_audio_ressource.get(), resampled_data.data(),
+                       static_cast<Uint32>(get_buffersize_bytes(resampled_data)));
     }
 }
+AudioRessource::operator SDL_AudioDeviceID() const {
+    return m_device_id;
+}
+
+AudioRessource::AudioRessource(std::string_view device, const SDL_AudioSpec& audio_spec) {
+    SDL_AudioSpec obtained{};
+    SDL_OpenAudioDevice(device.data(), SDL_AUDIO_PLAYBACK, &audio_spec, &obtained, 0);
+    assert(obtained.freq == audio_spec.freq && "Audio device configuration mismatch");
+    if (m_device_id == 0) {
+        spdlog::error("Failed to open audio: {}", SDL_GetError());
+        std::exit(EXIT_FAILURE);
+    }
+}
+
+AudioRessource::~AudioRessource() {
+    SDL_CloseAudioDevice(m_device_id);
+}
+
+SDL_AudioDeviceID AudioRessource::get() const {
+    return m_device_id;
+}

src/game-boy-emulator/audio.hpp

@@ -6,8 +6,28 @@
 #include <vector>
 class Emulator;
 
+/**
+ * Owner class for the SDL ressource AudioDeviceID.
+ */
+class AudioRessource {
+    SDL_AudioDeviceID m_device_id{};
+
+public:
+    explicit operator SDL_AudioDeviceID() const;
+    [[nodiscard]] SDL_AudioDeviceID get() const;
+
+    AudioRessource() = default;
+    AudioRessource(std::string_view device, const SDL_AudioSpec& audio_spec);
+    ~AudioRessource();
+    // Since we handle a ressource in this class delete copy operators.
+    AudioRessource(const AudioRessource&) = delete;
+    AudioRessource& operator=(const AudioRessource&) = delete;
+    AudioRessource(AudioRessource&&) = default;
+    AudioRessource& operator=(AudioRessource&&) = default;
+};
+
 class Audio {
-    SDL_AudioDeviceID m_device_id = 0;
+    AudioRessource m_audio_ressource;
     Resampler<SampleFrame> m_resampler;
 
     template <typename Container>
@@ -19,7 +39,5 @@ class Audio {
 
 public:
     explicit Audio(Emulator& emulator);
-    ~Audio();
-
     void callback(SampleFrame sample);
 };

src/game-boy-emulator/audiochannel.hpp

@@ -6,7 +6,7 @@ class AudioChannel {
 
     bool m_enabled = false;
 
-protected:
+
     // Registers of this channel
     uint8_t m_nrx0 = 0;
     uint8_t m_nrx1 = 0;
@@ -18,11 +18,11 @@ class AudioChannel {
     void set_enabled(bool enabled);
     [[nodiscard]] bool is_enabled() const;
 
-    virtual uint8_t read_nrx0() const;
-    virtual uint8_t read_nrx1() const;
-    virtual uint8_t read_nrx2() const;
-    virtual uint8_t read_nrx3() const;
-    virtual uint8_t read_nrx4() const;
+    [[nodiscard]] virtual uint8_t read_nrx0() const;
+    [[nodiscard]] virtual uint8_t read_nrx1() const;
+    [[nodiscard]] virtual uint8_t read_nrx2() const;
+    [[nodiscard]] virtual uint8_t read_nrx3() const;
+    [[nodiscard]] virtual uint8_t read_nrx4() const;
     virtual void set_nrx0(uint8_t value);
     virtual void set_nrx1(uint8_t value);
     virtual void set_nrx2(uint8_t value);
@@ -32,5 +32,10 @@ class AudioChannel {
     // Generate a sample in range 0..15
     virtual uint8_t get_sample() = 0;
 
+    AudioChannel() = default;
     virtual ~AudioChannel() = default;
+    AudioChannel(const AudioChannel&) = default;
+    AudioChannel(AudioChannel&&) = default;
+    AudioChannel& operator=(const AudioChannel&) = default;
+    AudioChannel& operator=(AudioChannel&&) = default;
 };

src/game-boy-emulator/cartridge.cpp

@@ -22,6 +22,7 @@ namespace {
 const int CARTRIDGE_TYPE_OFFSET = 0x147;
 const int TITLE_BEGIN = 0x134;
 const int TITLE_END = 0x143;
+const int TITLE_SIZE = TITLE_END - TITLE_BEGIN;
 } // namespace
 
 Cartridge::Cartridge(Emulator* emulator, std::vector<uint8_t> rom) :
@@ -107,7 +108,9 @@ void Cartridge::sync() {
 }
 
 std::string Cartridge::get_title(const std::vector<uint8_t>& rom) const {
-    auto title = std::string{rom.data() + TITLE_BEGIN, rom.data() + TITLE_END};
+    std::string title(TITLE_SIZE, '\0');
+    auto s = std::span(rom).subspan<TITLE_BEGIN, TITLE_SIZE>();
+    std::copy(s.begin(), s.end(), title.begin());
     std::erase_if(title, [](auto c) { return !std::isprint(c); });
     return title;
 }

src/game-boy-emulator/cartridge.hpp

@@ -17,6 +17,13 @@ class Cartridge {
 public:
     Cartridge(Emulator* emulator, std::vector<uint8_t> rom);
     ~Cartridge();
+    // Sadly, to keep the forward declarations for Mbc and MemoryMappedFile, a destructor for
+    // Cartridge is required to be defined. This triggers warnings about the rule of five, to
+    // silence those we have to manually define the other functions.
+    Cartridge(const Cartridge&) = delete;
+    Cartridge& operator=(const Cartridge&) = delete;
+    Cartridge(Cartridge&&) = default;
+    Cartridge& operator=(Cartridge&&) = default;
 
     [[nodiscard]] uint8_t read_byte(uint16_t address) const;
     void write_byte(uint16_t address, uint8_t value);

src/game-boy-emulator/cpu.cpp

@@ -75,7 +75,7 @@ void Cpu::step() {
         instructionADD(current_instruction, data);
         break;
     case opcodes::InstructionType::ADD_Signed:
-        instructionADD_Signed(data);
+        instructionADD_Signed(static_cast<int8_t>(data));
         break;
     case opcodes::InstructionType::NOP:
         break;
@@ -911,7 +911,7 @@ void Cpu::instructionADD(opcodes::Instruction instruction, uint16_t data) {
 void Cpu::instructionADD_Signed(int8_t data) {
     set_zero_flag(BitValues::Inactive);
     set_subtract_flag(BitValues::Inactive);
-    int result = static_cast<int>(registers.sp + data);
+    auto result = static_cast<int>(registers.sp + data);
     set_half_carry_flag(((registers.sp ^ data ^ (result & 0xFFFF)) & 0x10) == 0x10);
     set_carry_flag(((registers.sp ^ data ^ (result & 0xFFFF)) & 0x100) == 0x100);
     registers.sp = result;