Skip to content

Fixes profiler build and adds workflow #147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
LiquidityC merged 2 commits into from
Oct 13, 2025
Merged

Fixes profiler build and adds workflow #147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2ec4ed
Fixes profiler build and adds workflow
LiquidityC Oct 13, 2025
685fa46
Fix profiler build name
LiquidityC Oct 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions .github/workflows/cmake-multi-platform.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,14 @@ jobs:
# To add more build types (Release, Debug, RelWithDebInfo, etc.) customize the build_type list.
matrix:
platform:
- { name: Windows (MSVC), os: windows-latest, shell: sh, c_compiler: cl, cpp_compiler: cl, cmake_args: '-DSDLMIXER_VENDORED=ON -GNinja' }
- { name: Ubuntu (GCC), os: ubuntu-latest, shell: sh, c_compiler: gcc, cpp_compiler: g++, cmake_args: '-GNinja' }
- { name: Ubuntu (Clang), os: ubuntu-latest, shell: sh, c_compiler: clang, cpp_compiler: clang++, cmake_args: '-GNinja' }
- { name: Ubuntu (mingw), os: ubuntu-latest, shell: sh, c_compiler: mingw-w64-gcc, cpp_compiler: mingw-w64-g++,
- { name: Windows (MSVC), os: windows-latest, shell: sh, c_compiler: cl, cpp_compiler: cl, cmake_args: '-DSDLMIXER_VENDORED=ON -GNinja' }
- { name: Ubuntu (GCC), os: ubuntu-latest, shell: sh, c_compiler: gcc, cpp_compiler: g++, cmake_args: '-GNinja' }
- { name: 'Ubuntu (GCC, PROFILER)', os: ubuntu-latest, shell: sh, c_compiler: gcc, cpp_compiler: g++, cmake_args: '-DPROFILER=ON -GNinja' }
- { name: Ubuntu (Clang), os: ubuntu-latest, shell: sh, c_compiler: clang, cpp_compiler: clang++, cmake_args: '-GNinja' }
- { name: Ubuntu (mingw), os: ubuntu-latest, shell: sh, c_compiler: mingw-w64-gcc, cpp_compiler: mingw-w64-g++,
cmake_args: '-DSDLMIXER_VENDORED=ON -DSDLTTF_VENDORED=ON -DCMAKE_TOOLCHAIN_FILE=build_deps/toolchains/mingw-w64-x86_64.cmake -GNinja' }
- { name: Mac (GCC), os: macos-latest, shell: sh, c_compiler: gcc, cpp_compiler: g++, cmake_args: '-DSDLMIXER_VENDORED=ON -GNinja' }
- { name: Mac (Clang), os: macos-latest, shell: sh, c_compiler: clang, cpp_compiler: clang++, cmake_args: '-DSDLMIXER_VENDORED=ON -GNinja' }
- { name: Mac (GCC), os: macos-latest, shell: sh, c_compiler: gcc, cpp_compiler: g++, cmake_args: '-DSDLMIXER_VENDORED=ON -GNinja' }
- { name: Mac (Clang), os: macos-latest, shell: sh, c_compiler: clang, cpp_compiler: clang++, cmake_args: '-DSDLMIXER_VENDORED=ON -GNinja' }

steps:
- uses: actions/checkout@v4
Expand Down
2 changes: 1 addition & 1 deletion lib/profiler/src/profiler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#define PROFILER 1
#define PROFILER
Copy link

@coderabbitai coderabbitai bot Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Two concerns: hardcoded macro and potential breaking change.

  1. Hardcoded definition: Similar to rdtsc.c, the hardcoded #define PROFILER conflicts with the CI workflow's optional profiling builds. Consider defining this via CMake compiler flags instead.

  2. Macro semantics change: Removing the numeric value changes the macro from value-based (#define PROFILER 1) to presence-based (#define PROFILER). This is fine if all usage is #ifdef PROFILER, but could break code using #if PROFILER == 1 or similar value checks.

Run this script to verify how PROFILER is used throughout the codebase:

🏁 Script executed:

#!/bin/bash
# Check for potential breaking usage patterns

# Search for value-based checks that might break
rg -nP '#if\s+(PROFILER\s*(==|!=|>|<)|!PROFILER)' --type=c --type=cpp

# Show all PROFILER usage for context
rg -n -C2 'PROFILER' --type=c --type=cpp -g '!**/test/**'

Length of output: 2846

Remove hardcoded PROFILER definitions from source
Delete #define PROFILER from lib/profiler/src/{profiler.c, rdtsc.c, repetition_tester.c} and control profiling via build flags (e.g. add -DPROFILER in CMake).

🤖 Prompt for AI Agents 
In lib/profiler/src/profiler.c around line 1 there is a hardcoded preprocessor
flag; remove the line "#define PROFILER" from this file (and similarly from
rdtsc.c and repetition_tester.c) and instead control profiling via build-time
defines (e.g., add -DPROFILER to the CMake target compile options). Ensure no
other code relies on that in-source define and update CMakeLists.txt to
conditionally add -DPROFILER for targets that require profiling.


#include "common.h"
#include "profiler_c.h"
Expand Down
1 change: 1 addition & 0 deletions lib/profiler/src/rdtsc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include <sys/time.h>
#include <stdint.h>
#define PROFILER
Copy link

@coderabbitai coderabbitai bot Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Hardcoded PROFILER macro conflicts with optional profiling builds.

The hardcoded #define PROFILER means profiling is always enabled in this file, which conflicts with the CI workflow's approach of having both profiler-enabled and non-profiler builds. If profiling should be optional, this macro should be defined via CMake compiler flags (e.g., -DPROFILER) rather than hardcoded in the source.

Please verify:

  1. Whether rdtsc.h has conditional compilation that requires PROFILER to be defined
  2. If profiling should be optional or always-on in this file

Run this script to check how rdtsc.h uses the PROFILER macro:

If profiling should be optional, consider removing this hardcoded define and ensuring CMake passes -DPROFILER to the compiler when the PROFILER option is enabled.

🏁 Script executed:

#!/bin/bash
# Check if rdtsc.h has conditional compilation based on PROFILER

# Search for PROFILER usage in rdtsc.h
rg -n -C3 'PROFILER' lib/profiler/include/ lib/profiler/src/

Length of output: 2939

Remove hardcoded PROFILER macros from source files
Define PROFILER via compiler flags (e.g., CMake -DPROFILER) instead of #define in rdtsc.c, repetition_tester.c, profiler.c so that profiling can be enabled or disabled per build.

🤖 Prompt for AI Agents 
In lib/profiler/src/rdtsc.c around line 3 there is a hardcoded '#define
PROFILER'—remove that line and rely on defining PROFILER via compiler flags (for
example add -DPROFILER in your CMakeLists or build system) so profiling can be
enabled/disabled per build; ensure existing #ifdef PROFILER checks remain
unchanged and update the project build configuration to pass -DPROFILER when
profiling is desired (also remove the same hardcoded defines from
repetition_tester.c and profiler.c).

#include "rdtsc.h"

#ifdef __arm__
Expand Down
Loading