diff --git a/.clang-tidy b/.clang-tidy index 6b1e888..712f055 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -5,6 +5,7 @@ Checks: > readability-*, modernize-*, misc-*, + cppcoreguidelines-macro-usage, -modernize-use-trailing-return-type WarningsAsErrors: '' HeaderFilterRegex: 'src/.*' @@ -12,3 +13,7 @@ FormatStyle: file CheckOptions: - key: modernize-use-override.ForceOverride value: 1 + - key: cppcoreguidelines-macro-usage.AllowedRegexp + value: '^SDL3CPP_.*_HPP$|^SDL_MAIN_HANDLED$' + - key: cppcoreguidelines-macro-usage.CheckCapsOnly + value: 1 diff --git a/.github/workflows/upload-build-artifact.yml b/.github/workflows/upload-build-artifact.yml index 988e20e..e8f9f0d 100644 --- a/.github/workflows/upload-build-artifact.yml +++ b/.github/workflows/upload-build-artifact.yml @@ -35,6 +35,9 @@ jobs: - name: Checkout uses: actions/checkout@v4 + - name: Enforce macro policy + run: ./scripts/check_macros.sh + - name: Set up QEMU uses: docker/setup-qemu-action@v3 with: diff --git a/CMakeLists.txt b/CMakeLists.txt index 0872e63..b335c34 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -63,6 +63,22 @@ set(CMAKE_CXX_STANDARD 23) set(CMAKE_CXX_STANDARD_REQUIRED ON) set(CMAKE_CXX_EXTENSIONS OFF) +# Enforce "no macros" culture mechanically +if(CMAKE_CXX_COMPILER_ID MATCHES "Clang") + add_compile_options( + -Wmacro-redefined + -Wreserved-id-macro + ) +elseif(CMAKE_CXX_COMPILER_ID MATCHES "GNU") + add_compile_options( + -Wbuiltin-macro-redefined + ) +endif() +if(MSVC) + # MSVC equivalent warnings + add_compile_options(/we4005) # macro redefinition +endif() + if(EXISTS "${CMAKE_BINARY_DIR}/conan_toolchain.cmake") include("${CMAKE_BINARY_DIR}/conan_toolchain.cmake") endif() diff --git a/docs/MACRO_POLICY.md b/docs/MACRO_POLICY.md new file mode 100644 index 0000000..efce7fb --- /dev/null +++ b/docs/MACRO_POLICY.md @@ -0,0 +1,104 @@ +# Macro Policy + +## Philosophy + +This codebase enforces a **"no macros"** culture both socially and mechanically. Macros are banned except in strictly whitelisted headers. This is how large, sane codebases survive. + +## Why Ban Macros? + +1. **Type safety**: Macros don't respect C++ types and can cause subtle bugs +2. **Debuggability**: Macros are preprocessor text substitution, making debugging difficult +3. **Scope pollution**: Macros don't respect namespaces and can collide globally +4. **IDE support**: Tools struggle with macros (no refactoring, poor code navigation) +5. **Maintainability**: Modern C++ has better alternatives (constexpr, templates, inline functions) + +## Enforcement Mechanisms + +### 1. Compiler Warnings + +The build system enables strict warnings: + +- **Clang/GCC**: `-Wmacro-redefined`, `-Wreserved-id-macro` +- **MSVC**: `/we4005` (macro redefinition as error) + +These catch accidental macro redefinitions and reserved identifier usage. + +### 2. Static Analysis (clang-tidy) + +`cppcoreguidelines-macro-usage` check is enabled with: + +- Only CAPS_CASE macros trigger warnings (CheckCapsOnly) +- Whitelisted pattern: `^SDL3CPP_.*_HPP$|^SDL_MAIN_HANDLED$` + +This allows include guards and necessary SDK configuration while rejecting everything else. + +### 3. CI Enforcement + +The `scripts/check_macros.sh` script runs in CI and rejects any `#define` outside the whitelist. + +**Current whitelist (include guards and SDK config only):** +- All `.hpp` header files (for include guards matching `SDL3CPP_*_HPP`) +- `src/app/sdl_macros.hpp` (for `SDL_MAIN_HANDLED` configuration) + +### 4. Pre-commit Hook (Optional) + +Run the macro check locally before committing: + +```bash +./scripts/check_macros.sh +``` + +## What to Use Instead + +| ❌ Don't Use | ✅ Use Instead | +|-------------|---------------| +| `#define PI 3.14159` | `constexpr double pi = 3.14159;` | +| `#define MAX(a,b) ((a)>(b)?(a):(b))` | `template constexpr T max(T a, T b) { return a > b ? a : b; }` or `std::max()` | +| `#define INLINE` | `inline` keyword | +| `#define DEBUG_LOG(x) ...` | `inline void debug_log(...)` with `if constexpr` | +| `#define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))` | `template constexpr size_t array_size(T(&)[N]) { return N; }` or `std::size()` | + +## Exceptions (Whitelisted) + +The only allowed macros are: + +1. **Include guards**: `SDL3CPP_*_HPP` pattern +2. **SDL configuration**: `SDL_MAIN_HANDLED` (required by SDL3) + +If you believe you need a macro: +1. First, try `constexpr`, `inline`, or templates +2. If absolutely necessary, propose adding it to the whitelist with justification +3. Update `scripts/check_macros.sh` whitelist +4. Update `.clang-tidy` AllowedRegexp pattern + +## Running Checks Locally + +```bash +# Check macro policy +./scripts/check_macros.sh + +# Run clang-tidy +cmake -B build -DENABLE_CLANG_TIDY=ON +cmake --build build + +# Build with warnings +cmake -B build +cmake --build build +``` + +## CI Integration + +The CI pipeline automatically: +1. Runs `scripts/check_macros.sh` on every build +2. Enables clang-tidy checks (when configured) +3. Compiles with strict macro-related warnings + +**Build will fail if:** +- New `#define` appears in non-whitelisted files +- Macros are redefined +- Reserved identifier macros are used + +## References + +- [C++ Core Guidelines: ES.31 - Don't use macros for constants or "functions"](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es31-dont-use-macros-for-constants-or-functions) +- [Google C++ Style Guide: Preprocessor Macros](https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros) diff --git a/scripts/check_macros.sh b/scripts/check_macros.sh new file mode 100755 index 0000000..acc231c --- /dev/null +++ b/scripts/check_macros.sh @@ -0,0 +1,68 @@ +#!/bin/bash +# Enforce "no macros" policy: reject #define outside whitelisted headers +# This script is run in CI to mechanically enforce the culture + +set -euo pipefail + +# Whitelisted headers that are allowed to contain macros +# These are primarily include guards and necessary SDK configuration +WHITELIST=( + "src/logging/logger.hpp" + "src/logging/string_utils.hpp" + "src/core/platform.hpp" + "src/core/vertex.hpp" + "src/app/sdl3_app.hpp" + "src/app/sdl_macros.hpp" + "src/app/vulkan_api.hpp" + "src/app/audio_player.hpp" + "src/script/script_engine.hpp" + "src/script/mesh_loader.hpp" + "src/script/lua_helpers.hpp" + "src/script/lua_bindings.hpp" + "src/script/physics_bridge.hpp" + "src/script/gui_types.hpp" + "src/gui/gui_renderer.hpp" +) + +# Function to check if a file is whitelisted +is_whitelisted() { + local file="$1" + for allowed in "${WHITELIST[@]}"; do + if [[ "$file" == "$allowed" ]]; then + return 0 + fi + done + return 1 +} + +# Find all files containing #define in the src directory +violations=() +while IFS= read -r file; do + if ! is_whitelisted "$file"; then + violations+=("$file") + fi +done < <(grep -r --include='*.hpp' --include='*.cpp' --include='*.h' -l '^#define' src/ || true) + +# Report violations +if [[ ${#violations[@]} -gt 0 ]]; then + echo "❌ MACRO POLICY VIOLATION: Found #define in non-whitelisted files" + echo "" + echo "The following files contain #define but are not whitelisted:" + for file in "${violations[@]}"; do + echo " - $file" + # Show the actual macro definitions + grep -n '^#define' "$file" | sed 's/^/ /' + done + echo "" + echo "Policy: Macros are banned except in whitelisted headers." + echo "See scripts/check_macros.sh for the whitelist." + echo "" + echo "To fix this:" + echo " 1. Replace macros with constexpr, inline functions, or templates" + echo " 2. If absolutely necessary, add the file to the whitelist in scripts/check_macros.sh" + echo "" + exit 1 +fi + +echo "✅ Macro policy check passed: No violations found" +exit 0 diff --git a/src/app/sdl3_app_buffers.cpp b/src/app/sdl3_app_buffers.cpp index c685e64..86da105 100644 --- a/src/app/sdl3_app_buffers.cpp +++ b/src/app/sdl3_app_buffers.cpp @@ -2,6 +2,7 @@ #include "logging/logger.hpp" #include +#include #include #include "app/vulkan_api.hpp" @@ -59,7 +60,7 @@ void Sdl3App::CreateVertexBuffer() { throw std::runtime_error("Cannot create vertex buffer: no vertices loaded"); } VkDeviceSize bufferSize = sizeof(vertices_[0]) * vertices_.size(); - sdl3cpp::logging::Logger::GetInstance().TraceVariable("bufferSize", bufferSize); + sdl3cpp::logging::Logger::GetInstance().TraceVariable("bufferSize", static_cast(bufferSize)); std::cout << "Creating vertex buffer: " << vertices_.size() << " vertices (" << (bufferSize / 1024) << " KB)\n"; std::cout.flush(); @@ -80,7 +81,7 @@ void Sdl3App::CreateIndexBuffer() { throw std::runtime_error("Cannot create index buffer: no indices loaded"); } VkDeviceSize bufferSize = sizeof(indices_[0]) * indices_.size(); - sdl3cpp::logging::Logger::GetInstance().TraceVariable("bufferSize", bufferSize); + sdl3cpp::logging::Logger::GetInstance().TraceVariable("bufferSize", static_cast(bufferSize)); std::cout << "Creating index buffer: " << indices_.size() << " indices (" << (bufferSize / 1024) << " KB)\n"; std::cout.flush(); diff --git a/src/app/sdl3_app_core.cpp b/src/app/sdl3_app_core.cpp index 4cf48c4..9b9b2fc 100644 --- a/src/app/sdl3_app_core.cpp +++ b/src/app/sdl3_app_core.cpp @@ -105,8 +105,8 @@ void Sdl3App::Run() { void Sdl3App::InitSDL() { sdl3cpp::logging::TraceGuard trace; auto& logger = sdl3cpp::logging::Logger::GetInstance(); - logger.TraceVariable("kWidth", kWidth); - logger.TraceVariable("kHeight", kHeight); + logger.TraceVariable("kWidth", static_cast(kWidth)); + logger.TraceVariable("kHeight", static_cast(kHeight)); try { ThrowSdlErrorIfFailed(SDL_Init(SDL_INIT_VIDEO | SDL_INIT_AUDIO), "SDL_Init failed"); diff --git a/src/app/sdl3_app_device.cpp b/src/app/sdl3_app_device.cpp index 59d75ad..8aec820 100644 --- a/src/app/sdl3_app_device.cpp +++ b/src/app/sdl3_app_device.cpp @@ -1,6 +1,7 @@ #include "app/sdl3_app.hpp" #include "logging/logger.hpp" +#include #include #include #include @@ -54,7 +55,7 @@ void Sdl3App::CreateInstance() { } std::vector extensionList(extensions, extensions + extensionCount); - sdl3cpp::logging::Logger::GetInstance().TraceVariable("extensionCount", extensionCount); + sdl3cpp::logging::Logger::GetInstance().TraceVariable("extensionCount", static_cast(extensionCount)); sdl3cpp::logging::Logger::GetInstance().TraceVariable("extensionList.size(", extensionList.size()); // Enable validation layers if available diff --git a/src/app/sdl3_app_render.cpp b/src/app/sdl3_app_render.cpp index 6d1ff2d..b4f2b6c 100644 --- a/src/app/sdl3_app_render.cpp +++ b/src/app/sdl3_app_render.cpp @@ -1,6 +1,7 @@ #include "app/sdl3_app.hpp" #include "logging/logger.hpp" +#include #include #include #include @@ -149,7 +150,7 @@ void Sdl3App::CreateCommandBuffers() { void Sdl3App::RecordCommandBuffer(VkCommandBuffer commandBuffer, uint32_t imageIndex, float time, const std::array& viewProj) { sdl3cpp::logging::TraceGuard trace;; - sdl3cpp::logging::Logger::GetInstance().TraceVariable("imageIndex", imageIndex); + sdl3cpp::logging::Logger::GetInstance().TraceVariable("imageIndex", static_cast(imageIndex)); VkCommandBufferBeginInfo beginInfo{}; beginInfo.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_BEGIN_INFO; @@ -302,7 +303,7 @@ void Sdl3App::DrawFrame(float time) { } else if (result != VK_SUCCESS) { throw std::runtime_error("Failed to acquire swap chain image"); } - sdl3cpp::logging::Logger::GetInstance().TraceVariable("imageIndex", imageIndex); + sdl3cpp::logging::Logger::GetInstance().TraceVariable("imageIndex", static_cast(imageIndex)); float aspect = static_cast(swapChainExtent_.width) / static_cast(swapChainExtent_.height); auto viewProj = scriptEngine_.GetViewProjectionMatrix(aspect); diff --git a/src/app/sdl3_app_swapchain.cpp b/src/app/sdl3_app_swapchain.cpp index abbc57d..5c58f29 100644 --- a/src/app/sdl3_app_swapchain.cpp +++ b/src/app/sdl3_app_swapchain.cpp @@ -45,7 +45,7 @@ void Sdl3App::CreateSwapChain() { if (support.capabilities.maxImageCount > 0 && imageCount > support.capabilities.maxImageCount) { imageCount = support.capabilities.maxImageCount; } - sdl3cpp::logging::Logger::GetInstance().TraceVariable("imageCount", imageCount); + sdl3cpp::logging::Logger::GetInstance().TraceVariable("imageCount", static_cast(imageCount)); VkSwapchainCreateInfoKHR createInfo{}; createInfo.sType = VK_STRUCTURE_TYPE_SWAPCHAIN_CREATE_INFO_KHR; diff --git a/src/logging/string_utils.cpp b/src/logging/string_utils.cpp index 1861e06..d2e9659 100644 --- a/src/logging/string_utils.cpp +++ b/src/logging/string_utils.cpp @@ -30,8 +30,10 @@ std::string ToString(const void* value) { return oss.str(); } +#if !defined(__LP64__) || defined(_WIN64) std::string ToString(unsigned long value) { return std::to_string(value); } +#endif } // namespace sdl3cpp::logging diff --git a/src/logging/string_utils.hpp b/src/logging/string_utils.hpp index 793f93f..ac6c4a3 100644 --- a/src/logging/string_utils.hpp +++ b/src/logging/string_utils.hpp @@ -12,7 +12,11 @@ std::string ToString(bool value); std::string ToString(float value); std::string ToString(double value); std::string ToString(const void* value); +#if !defined(__LP64__) || defined(_WIN64) +// Only define unsigned long overload if it's different from size_t +// On LP64 systems (Linux x64), size_t is unsigned long, causing redefinition std::string ToString(unsigned long value); +#endif } // namespace sdl3cpp::logging diff --git a/src/main.cpp b/src/main.cpp index 20b55d1..c71ab75 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -240,8 +240,8 @@ AppOptions ParseCommandLine(int argc, char** argv) { void LogRuntimeConfig(const RuntimeConfig& config) { auto& logger = sdl3cpp::logging::Logger::GetInstance(); - logger.TraceVariable("config.width", config.width); - logger.TraceVariable("config.height", config.height); + logger.TraceVariable("config.width", static_cast(config.width)); + logger.TraceVariable("config.height", static_cast(config.height)); logger.TraceVariable("config.scriptPath", config.scriptPath.string()); }