Removal of import std#19
Conversation
|
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 due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR converts module units to global module fragments with explicit standard headers, migrates unit tests from Boost.UT to doctest, updates math constants and some math implementations, and adjusts CMake presets, compiler flags, and third-party/static BGFX build settings. ChangesC++ Module Refactoring & Test Framework Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 3
🧹 Nitpick comments (1)
engine/native/core/math/constants.cppm (1)
28-29: ⚡ Quick win
UINT32_MAX_D/UINT32_MAX_Fare reciprocals, not max values — rename to avoid confusion.Both constants hold
1.0 / std::numeric_limits<uint32_t>::max()≈ 2.33×10⁻¹⁰. The nameUINT32_MAX_Dis a homonym for the C macroUINT32_MAX(4294967295), so any caller reading the name without checking the definition may silently use it as if it holds the max value instead of its reciprocal.♻️ Suggested rename
- constexpr double UINT32_MAX_D = 1. / std::numeric_limits<uint32_t>::max(); - constexpr float UINT32_MAX_F = 1.f / std::numeric_limits<uint32_t>::max(); + constexpr double INV_UINT32_MAX_D = 1. / std::numeric_limits<uint32_t>::max(); + constexpr float INV_UINT32_MAX_F = 1.f / std::numeric_limits<uint32_t>::max();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/core/math/constants.cppm` around lines 28 - 29, The constants UINT32_MAX_D and UINT32_MAX_F are actually the reciprocal of the uint32 max (1.0 / max) and should be renamed to avoid confusion; rename them to a clearer identifier such as UINT32_INV_MAX_D and UINT32_INV_MAX_F (or UINT32_RECIPROCAL_MAX_D/F) in constants.cppm and update every usage site to the new names (search for UINT32_MAX_D and UINT32_MAX_F), keeping the same types and values and updating any comments/docstrings to match the new meaning so callers no longer mistake them for the maximum value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@engine/native/core/definitions/definitions.cppm`:
- Around line 1-3: The code uses std::is_arithmetic_v, std::is_trivial_v and
std::false_type but only includes <concepts>; add the missing <type_traits>
include at the top of the module unit so these type-trait identifiers are
declared (i.e., add the `#include` for <type_traits> alongside the existing
includes in the module preamble to ensure std::is_arithmetic_v,
std::is_trivial_v and std::false_type are available).
In `@engine/native/core/math/math.test.cpp`:
- Around line 7-9: The test calls draco::math::pow(2., .5) and compares it with
draco::math::SQRT2 using REQUIRE(result == expected), which can fail due to pow
not being correctly rounded; change the test to compute the left-hand side with
std::sqrt(2.0) (or draco::math::sqrt if you provide a correctly-rounded wrapper)
instead of draco::math::pow, or keep pow but replace the exact equality check
with a tolerance check (e.g., use Catch's Approx) comparing result to
std::numbers::sqrt2_v<double> or draco::math::SQRT2 to avoid 1-ULP spurious
failures.
In `@engine/native/core/math/vector4.cppm`:
- Line 99: The commented-out static_assert removed the only compile-time
guarantee that Vector4 is standard-layout, leaving the non-consteval operator[]
(which uses (&x)[i]) undefined behavior if the type changes; restore the
static_assert for std::is_standard_layout_v<Vector4> to re-establish the
invariant, or alternatively change the non-consteval operator[] implementation
in Vector4 to mirror the consteval path by using explicit member selection (x,
y, z, w) or a switch/array of pointers to members instead of (&x)[i], and add a
brief comment documenting the chosen approach.
---
Nitpick comments:
In `@engine/native/core/math/constants.cppm`:
- Around line 28-29: The constants UINT32_MAX_D and UINT32_MAX_F are actually
the reciprocal of the uint32 max (1.0 / max) and should be renamed to avoid
confusion; rename them to a clearer identifier such as UINT32_INV_MAX_D and
UINT32_INV_MAX_F (or UINT32_RECIPROCAL_MAX_D/F) in constants.cppm and update
every usage site to the new names (search for UINT32_MAX_D and UINT32_MAX_F),
keeping the same types and values and updating any comments/docstrings to match
the new meaning so callers no longer mistake them for the maximum value.
🪄 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: 8561d66f-d6f4-4132-b860-0a29c6e7afb5
📒 Files selected for processing (13)
CMakeLists.txtCMakePresets.jsoncmake/Compiler.cmakecmake/Modules.cmakeengine/native/CMakeLists.txtengine/native/core/definitions/definitions.cppmengine/native/core/definitions/version.cppmengine/native/core/math/constants.cppmengine/native/core/math/math.cppmengine/native/core/math/math.test.cppengine/native/core/math/vector4.cppmengine/native/thirdparty/CMakeLists.txtengine/native/thirdparty/doctest/doctest.h
💤 Files with no reviewable changes (2)
- CMakeLists.txt
- cmake/Compiler.cmake
| module; | ||
|
|
||
| #include <concepts> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does the C++23 standard require <concepts> to transitively include <type_traits>? Where are std::is_arithmetic_v, std::is_trivial_v, and std::false_type declared?
💡 Result:
No, the C++23 standard does not require <concepts> to transitively include <type_traits>. The header synopsis for <concepts> in the standard draft ([1][2]) defines concepts like same_as, derived_from, integral, etc., but does not include or require declarations from <type_traits> such as type traits or integral_constant (from which false_type derives). Header inclusions are not mandated by the standard beyond the synopsis; implementations may choose transitive includes for convenience, but it is not required.[3][1][2][4] std::is_arithmetic_v, std::is_trivial_v are declared in <type_traits> ([5][6][7][8]). std::false_type is declared in <type_traits> as it inherits from std::integral_constant<bool, false>, which is part of the metaprogramming types in that header ([6][9][10]).
Citations:
- 1: https://eel.is/c++draft/concepts.syn
- 2: https://en.cppreference.com/w/cpp/header/concepts.html
- 3: https://en.cppreference.com/w/cpp/header/concepts
- 4: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/n4951.html
- 5: https://en.cppreference.com/w/cpp/types/is_arithmetic
- 6: https://docs.cppreference.com/w/cpp/header/type_traits.html
- 7: https://cppreference.dev/w/cpp/types/is_trivial
- 8: https://en.cppreference.com/w/cpp/header/type_traits
- 9: https://www.ece.uvic.ca/~frodo/cppbook/cppdraft/n4861/meta.type.synop
- 10: https://timsong-cpp.github.io/cppwp/meta.type.synop
Add missing <type_traits> header — type-trait identifiers used without declaration.
This module uses std::is_arithmetic_v (line 13), std::is_trivial_v (line 16), and std::false_type (line 22), all declared in <type_traits> according to the C++ standard. The <concepts> header does not declare these identifiers, and the standard does not require <concepts> to transitively include <type_traits>. While some implementations include it transitively for convenience, relying on this is fragile and will fail on stricter implementations like MSVC STL. Since the PR goal is to replace import std; with precise STL headers, this include must be added.
🔧 Proposed fix
module;
`#include` <concepts>
+#include <type_traits>
export module core.defs;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| module; | |
| #include <concepts> | |
| module; | |
| `#include` <concepts> | |
| `#include` <type_traits> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@engine/native/core/definitions/definitions.cppm` around lines 1 - 3, The code
uses std::is_arithmetic_v, std::is_trivial_v and std::false_type but only
includes <concepts>; add the missing <type_traits> include at the top of the
module unit so these type-trait identifiers are declared (i.e., add the `#include`
for <type_traits> alongside the existing includes in the module preamble to
ensure std::is_arithmetic_v, std::is_trivial_v and std::false_type are
available).
There was a problem hiding this comment.
Yes, it's a transitive include.
There was a problem hiding this comment.
@OldDev78, understood — on GCC (libstdc++) and Clang (libc++), <concepts> does pull in <type_traits> transitively in practice. Since the project exclusively targets GCC/Clang (as seen in cmake/Compiler.cmake), this holds. Just worth noting it won't work on MSVC STL, but given the targeted toolchain that's a valid conscious decision.
🐇 curious nibble
🧠 Learnings used
Learnt from: mcdubhghlas
Repo: Redot-Engine/DraconicEngine PR: 1
File: core/math/vector4.cppm:136-140
Timestamp: 2026-03-17T14:51:36.674Z
Learning: For all C++ module files (.cppm), ensure the build flags cover AVX2/-mfma unconditionally (as in the project: -mavx2 -mfma). If you use BMI1/BMI2 intrinsics in a module, also enable them explicitly with -mbmi and -mbmi2, since BMI features are not guaranteed by AVX2 alone. Note that SSE3 and older SSEs are guaranteed via ISA backwards compatibility on AVX2 targets, so no SSE2/SSE3 fallbacks are needed.
| double result = draco::math::pow(2., .5); | ||
| constexpr double expected = draco::math::SQRT2; | ||
| REQUIRE(result == expected); |
There was a problem hiding this comment.
REQUIRE(result == expected) may be unreliable — std::pow(2., 0.5) is not guaranteed to match sqrt2_v<double> exactly.
std::numbers::sqrt2_v<double> is the correctly-rounded IEEE 754 double approximation of √2. std::pow(2., 0.5) goes through the general pow implementation, which — unlike sqrt — is not required by IEEE 754 or the C++ standard to be correctly rounded. On some libm implementations, pow(2., 0.5) may return a value that is 1 ULP off from sqrt2_v<double>, causing this test to fail spuriously.
Use std::sqrt(2.) (which is correctly rounded) or a tolerance-based check:
🐛 Proposed fix
TEST_CASE("pow") {
- double result = draco::math::pow(2., .5);
- constexpr double expected = draco::math::SQRT2;
- REQUIRE(result == expected);
+ double result = draco::math::pow(2., .5);
+ constexpr double expected = draco::math::SQRT2;
+ // pow is not required to be correctly-rounded; allow 1 ULP of slack
+ REQUIRE(result == doctest::Approx(expected).epsilon(1e-15));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| double result = draco::math::pow(2., .5); | |
| constexpr double expected = draco::math::SQRT2; | |
| REQUIRE(result == expected); | |
| double result = draco::math::pow(2., .5); | |
| constexpr double expected = draco::math::SQRT2; | |
| // pow is not required to be correctly-rounded; allow 1 ULP of slack | |
| REQUIRE(result == doctest::Approx(expected).epsilon(1e-15)); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@engine/native/core/math/math.test.cpp` around lines 7 - 9, The test calls
draco::math::pow(2., .5) and compares it with draco::math::SQRT2 using
REQUIRE(result == expected), which can fail due to pow not being correctly
rounded; change the test to compute the left-hand side with std::sqrt(2.0) (or
draco::math::sqrt if you provide a correctly-rounded wrapper) instead of
draco::math::pow, or keep pow but replace the exact equality check with a
tolerance check (e.g., use Catch's Approx) comparing result to
std::numbers::sqrt2_v<double> or draco::math::SQRT2 to avoid 1-ULP spurious
failures.
| static_assert(alignof(Vector4) == 16, "Vector4 must be 16-byte aligned"); | ||
| static_assert(trivial<Vector4>, "Vector4 must be trivial"); | ||
| static_assert(std::is_standard_layout_v<Vector4>, "Vector4 must be standard layout"); | ||
| //static_assert(std::is_standard_layout_v<Vector4>, "Vector4 must be standard layout"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there is any other explanation or recent changes around the standard_layout assert
rg -n "standard_layout" --type cpp -C3
rg -n "is_standard_layout" --type cpp -C3Repository: Redot-Engine/DraconicEngine
Length of output: 53
🏁 Script executed:
# First, locate the file
find . -name "vector4.cppm" -type fRepository: Redot-Engine/DraconicEngine
Length of output: 110
🏁 Script executed:
# Read the file to check line 99 and surrounding context
# Get file info first
wc -l engine/native/core/math/vector4.cppmRepository: Redot-Engine/DraconicEngine
Length of output: 112
🏁 Script executed:
# Read the full file to understand structure
cat -n engine/native/core/math/vector4.cppm | head -150Repository: Redot-Engine/DraconicEngine
Length of output: 5729
🏁 Script executed:
# Search for the static_assert and operator[] in the cppm file
rg "static_assert|is_standard_layout|operator\[\]" engine/native/core/math/vector4.cppm -n -C 3Repository: Redot-Engine/DraconicEngine
Length of output: 1078
Commented-out is_standard_layout assert removes the only guard for (&x)[i] pointer arithmetic.
The non-consteval branch of operator[] (lines 36, 48) uses (&x)[i] to index struct members, which is only well-defined if the type is standard layout. The static_assert at line 99 was the sole compile-time guarantee for this invariant, and removing it without explanation leaves the runtime path vulnerable to UB if the struct property ever changes.
Either restore the assert and address any failure, or add a comment explaining why it was removed and replace (&x)[i] with explicit member selection (as in the consteval path).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@engine/native/core/math/vector4.cppm` at line 99, The commented-out
static_assert removed the only compile-time guarantee that Vector4 is
standard-layout, leaving the non-consteval operator[] (which uses (&x)[i])
undefined behavior if the type changes; restore the static_assert for
std::is_standard_layout_v<Vector4> to re-establish the invariant, or
alternatively change the non-consteval operator[] implementation in Vector4 to
mirror the consteval path by using explicit member selection (x, y, z, w) or a
switch/array of pointers to members instead of (&x)[i], and add a brief comment
documenting the chosen approach.
JoltedJon
left a comment
JoltedJon
left a comment
There was a problem hiding this comment.
From a code review standpoint looks good to me. Not at computer so can't compile it right now
Please test with different clang versions (>18) if possible. Next, I'll specify clang as the compiler. |
2 participants
Remove import std; and use STL headers in global module fragment instead.
Summary by CodeRabbit