Fix max/min compile errors#11
Conversation
|
Unfortunately, no time available right now to debug the |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe pull request addresses compilation issues on modern Ubuntu systems by expanding CI build dependencies with additional X11 development packages, and introducing C++ Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@kernel/misc.h`:
- Around line 43-50: Replace the incorrect <cmath> include with the correct
<algorithm> header and change the two hard-coded long overloads to constexpr
function templates so min and max work with any comparable type used across the
codebase; specifically update the definitions of min and max (currently declared
as constexpr const long& min(const long& a, const long& b) and max(...)) to
template<typename T> constexpr const T& min(const T& a, const T& b) and
template<typename T> constexpr const T& max(const T& a, const T& b) and ensure
these use std::min/std::max from <algorithm> inside the `#ifdef` __cplusplus
block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2164ff4-41ed-4e49-a891-84a71b5cd413
📒 Files selected for processing (2)
.github/workflows/c-cpp.ymlkernel/misc.h
| #ifdef __cplusplus | ||
| #include <cmath> | ||
| constexpr const long& min(const long& a, const long& b) { | ||
| return std::min(a, b); | ||
| } | ||
| constexpr const long& max(const long& a, const long& b) { | ||
| return std::max(a, b); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What header file defines std::min and std::max in C++?
💡 Result:
The header file that defines std::min and std::max in C++ is <algorithm>.
Citations:
- 1: https://en.cppreference.com/w/cpp/algorithm/min.html
- 2: https://xn--fiq.cppreference.com/w/cpp/algorithm/max.html
- 3: https://en.cppreference.com/w/cpp/algorithm/min
- 4: https://en.cppreference.com/w/cpp/algorithm/minmax.html
- 5: https://wwww.cppreference.com/w/cpp/algorithm/max.html
🏁 Script executed:
# Search for min and max function calls with int arguments in the mentioned files
rg '\bmin\s*\(' -A 2 -B 2 --type cpp | grep -E "(int|min)" | head -40Repository: brentjohnson/xconq
Length of output: 2858
🏁 Script executed:
# Check specific files mentioned in the review
for file in sdlscreen.cc aiunit.cc aiunit2.cc; do
if git ls-files | grep -q "$file"; then
echo "=== $file ==="
rg '\b(min|max)\s*\(' "$file" -A 1 -B 1 | head -20
fi
doneRepository: brentjohnson/xconq
Length of output: 290
🏁 Script executed:
# Verify the current state of kernel/misc.h around lines 43-50
cat -n kernel/misc.h | sed -n '40,55p'Repository: brentjohnson/xconq
Length of output: 534
Wrong header: <cmath> does not define std::min/std::max.
std::min and std::max are defined in <algorithm>, not <cmath>. This will fail to compile.
Additionally, the functions only accept const long&, but the codebase extensively uses min/max with int arguments throughout (e.g., sdl/sdlscreen.cc, src/gdl/gamearea/area.cc, sdl/sdlmap.cc). Consider using function templates to preserve type flexibility like the original macros.
Proposed fix
`#ifdef` __cplusplus
-#include <cmath>
-constexpr const long& min(const long& a, const long& b) {
+#include <algorithm>
+template<typename T>
+constexpr const T& min(const T& a, const T& b) {
return std::min(a, b);
}
-constexpr const long& max(const long& a, const long& b) {
+template<typename T>
+constexpr const T& max(const T& a, const T& b) {
return std::max(a, b);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@kernel/misc.h` around lines 43 - 50, Replace the incorrect <cmath> include
with the correct <algorithm> header and change the two hard-coded long overloads
to constexpr function templates so min and max work with any comparable type
used across the codebase; specifically update the definitions of min and max
(currently declared as constexpr const long& min(const long& a, const long& b)
and max(...)) to template<typename T> constexpr const T& min(const T& a, const
T& b) and template<typename T> constexpr const T& max(const T& a, const T& b)
and ensure these use std::min/std::max from <algorithm> inside the `#ifdef`
__cplusplus block.
There was a problem hiding this comment.
Good catch regarding <algorithm> (though the code did compile). No joy with the template definition though. Apparently, the problem is that a and b can be of different types but there's no std::min() / std::max() implementation for every type pair.
There was a problem hiding this comment.
@kazarmy, good point — std::min/std::max require both arguments to be the same type, so a single template<typename T> would fail to deduce T for mixed-type calls like min(int_val, long_val).
The minimal correct fix is simply changing <cmath> to <algorithm>. The existing const long& overloads already handle mixed-type calls gracefully via implicit numeric conversion to long, which mirrors what the original macros did.
If full type-safety for mixed-type pairs is ever desired in the future, a two-type-parameter template with std::common_type returning by value would work:
`#ifdef` __cplusplus
`#include` <algorithm>
`#include` <type_traits>
template<typename T, typename U>
constexpr typename std::common_type<T, U>::type min(const T& a, const U& b) {
typedef typename std::common_type<T, U>::type C;
return std::min(static_cast<C>(a), static_cast<C>(b));
}
template<typename T, typename U>
constexpr typename std::common_type<T, U>::type max(const T& a, const U& b) {
typedef typename std::common_type<T, U>::type C;
return std::max(static_cast<C>(a), static_cast<C>(b));
}
`#else`But that's a larger change than needed here. For the immediate goal, just swapping <cmath> → <algorithm> is sufficient and correct.
✏️ Learnings added
Learnt from: kazarmy
Repo: brentjohnson/xconq PR: 11
File: kernel/misc.h:43-50
Timestamp: 2026-03-29T13:23:55.324Z
Learning: In `kernel/misc.h` of the `brentjohnson/xconq` repository, `min`/`max` are defined as `constexpr const long&` overloads forwarding to `std::min`/`std::max` from `<algorithm>`. A single-type-parameter template cannot be used because `min`/`max` are called with arguments of different numeric types throughout the codebase (e.g., `int` and `long`). The `long` overloads handle mixed-type calls via implicit numeric conversion.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
1 participant
This pr fixes #10 by fixing the max/min compile errors reported there. Tested on Ubuntu 24.04 with g++ 13.3.0.
Summary by CodeRabbit