Fixes profiler build and adds workflow #147
Conversation
WalkthroughAdds an Ubuntu GCC PROFILER CI job and normalizes workflow matrix formatting. Removes the numeric assignment from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/cmake-multi-platform.yml (1)
116-116: Consider including profiler build in tests.The test step currently excludes mingw but doesn't explicitly consider the new PROF configuration. Verify whether profiler-enabled builds should run tests, or if there's a reason to exclude them.
If profiler builds should also run tests, consider updating the condition:
- if: ${{ matrix.platform.os == 'ubuntu-latest' && matrix.platform.c_compiler != 'mingw-w64-gcc' }} + if: ${{ matrix.platform.os == 'ubuntu-latest' && matrix.platform.c_compiler != 'mingw-w64-gcc' }}(The condition appears correct as-is, since it checks the compiler type rather than the platform name, so PROF builds should already be included. This comment is just to confirm the intent.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/cmake-multi-platform.yml(1 hunks)lib/profiler/src/profiler.c(1 hunks)lib/profiler/src/rdtsc.c(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Ubuntu (mingw)
- GitHub Check: Ubuntu (GCC
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Ubuntu (Clang)
- GitHub Check: Mac (GCC)
- GitHub Check: Mac (Clang)
- GitHub Check: Ubuntu (GCC)
- GitHub Check: Windows (MSVC)
🔇 Additional comments (1)
.github/workflows/cmake-multi-platform.yml (1)
34-34: Approve profiler-enabled CI job
The Ubuntu (GCC, PROF) entry with-DPROFILER=ONadds valuable coverage, and CMakeLists.txt already defines PROFILER as a cache option and applies-DPROFILERviaadd_definitionswhen enabled.
| @@ -1,4 +1,4 @@ | |||
| #define PROFILER 1 | |||
| #define PROFILER | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Two concerns: hardcoded macro and potential breaking change.
-
Hardcoded definition: Similar to
rdtsc.c, the hardcoded#define PROFILERconflicts with the CI workflow's optional profiling builds. Consider defining this via CMake compiler flags instead. -
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 == 1or 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.
| @@ -1,5 +1,6 @@ | |||
| #include <sys/time.h> | |||
| #include <stdint.h> | |||
| #define PROFILER | |||
There was a problem hiding this comment.
🛠️ 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:
- Whether
rdtsc.hhas conditional compilation that requires PROFILER to be defined - 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).
Summary by CodeRabbit