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 (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughConvert C++ module units to global fragments with explicit standard headers, migrate tests from Boost.UT to doctest, update math constants/use of PI, and adjust CMake presets, compiler flags, third‑party BGFX static settings, and PIC handling. ChangesC++ Module Refactoring & Test Framework Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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
| 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.
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. |
JoltedJon
left a comment
JoltedJon
left a comment
There was a problem hiding this comment.
Tested with Clang 22.1.5 works well
AR-DEV-1
left a comment
AR-DEV-1
left a comment
There was a problem hiding this comment.
Tested with Clang 20 & works as expected
5 participants
Remove import std; and use STL headers in global module fragment instead.
Summary by CodeRabbit