Add common types into definitions#23
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughAdds an exported module ChangesType aliases definition and adoption
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)
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: 1
🧹 Nitpick comments (1)
engine/native/core/definitions/definitions.cppm (1)
26-26: Consider consistency:isizeshould matchusizesemantics.The
isizetype is hardcoded toint64_t, whileusizeis defined asstd::size_t. Since the project explicitly targets 64-bit platforms (as evidenced by x86_64-only SIMD configuration incmake/Compiler.cmakeand C++23 requirements), the fixed 64-bit size is intentional. However, for semantic consistency with Rust'sisize/usizepairing where both are pointer-sized, consider usingstd::intptr_tforisizeto matchusize's pointer-awareness, even if the project currently targets only 64-bit systems.🤖 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` at line 26, The alias isize is currently fixed to int64_t which mismatches the pointer-sized semantics of usize; change the isize typedef/using to use a pointer-sized integer type (std::intptr_t) instead of int64_t so isize mirrors usize's pointer-awareness; update the using declaration for isize to std::intptr_t and ensure the header includes <cstdint> (or already provides intptr_t) so the symbol is available.
🤖 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`:
- Line 4: The code uses std::size_t in definitions.cppm (around the use on line
27) but only includes <cstdint>; add the missing header <cstddef> to the
includes at the top of definitions.cppm so std::size_t is defined portably;
update the include list (near the existing `#include` <cstdint>) to also include
<cstddef> so any use of std::size_t in functions or types in this module is
standard-compliant.
---
Nitpick comments:
In `@engine/native/core/definitions/definitions.cppm`:
- Line 26: The alias isize is currently fixed to int64_t which mismatches the
pointer-sized semantics of usize; change the isize typedef/using to use a
pointer-sized integer type (std::intptr_t) instead of int64_t so isize mirrors
usize's pointer-awareness; update the using declaration for isize to
std::intptr_t and ensure the header includes <cstdint> (or already provides
intptr_t) so the symbol is available.
🪄 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: 50a41540-2054-44d9-b008-3792246df415
📒 Files selected for processing (1)
engine/native/core/definitions/definitions.cppm
| using i8 = int8_t; | ||
| using i16 = int16_t; | ||
| using i32 = int32_t; | ||
| using i64 = int64_t; | ||
|
|
||
| using uint = unsigned int; | ||
| using u8 = uint8_t; | ||
| using u16 = uint16_t; | ||
| using u32 = uint32_t; | ||
| using u64 = uint64_t; | ||
|
|
||
| using f32 = float; | ||
| using f64 = double; | ||
|
|
||
| using isize = int64_t; | ||
| using usize = std::size_t; | ||
|
|
||
| using rawptr = void*; | ||
| using uintptr = uintptr_t; | ||
| using ptrdiff = ptrdiff_t; | ||
|
|
||
| // UTF-32 type | ||
| using rune = u32; |
There was a problem hiding this comment.
core.version also depends on the common types, would you mind moving these declarations to a core.stdtypes module?
Conversely, maybe core.version can also import this and move up instead...
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/version.cppm`:
- Around line 10-16: The Version struct and its fields (struct Version, members
major, minor, patch) currently use signed i16 which allows negative values and
halves the positive range; change the types of major, minor, and patch to the
unsigned 16-bit alias (u16 or std::uint16_t per project aliases) so the exported
contract preserves the full positive range, and keep the constexpr VERSION
initialization unchanged to use the new unsigned fields.
🪄 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: 8229fb21-61f1-483e-9444-2ac81cd32a08
📒 Files selected for processing (4)
engine/native/core/definitions/definitions.cppmengine/native/core/definitions/stdtypes.cppmengine/native/core/definitions/version.cppmengine/native/core/math/constants.cppm
✅ Files skipped from review due to trivial changes (1)
- engine/native/core/definitions/stdtypes.cppm
🚧 Files skipped from review as they are similar to previous changes (1)
- engine/native/core/definitions/definitions.cppm
AR-DEV-1
left a comment
AR-DEV-1
left a comment
There was a problem hiding this comment.
LGTM & is definitely a good change
Migrate HEAD:master to stdtypes
JoltedJon
dismissed stale reviews from OldDev78 and Arctis-Fireblight
via
af6dc32
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/version.cppm`:
- Around line 21-23: The parse implementation for formatter::parse currently
returns ctx.begin() unconditionally; update the parse(std::format_parse_context
&ctx) in version.cppm to validate the format-spec range: if ctx.begin() ==
ctx.end() return ctx.begin() (or ctx.end()) to accept an empty spec, otherwise
consume/validate allowed specifiers or throw std::format_error for any
non-empty/unsupported spec; ensure you use std::format_error and return the
appropriate iterator as required by C++23.
🪄 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: a31df24b-8c22-478a-aaf5-7263fdf302e3
📒 Files selected for processing (4)
engine/native/core/definitions/definitions.cppmengine/native/core/definitions/stdtypes.cppmengine/native/core/definitions/version.cppmengine/native/core/math/constants.cppm
| using f32 = float; | ||
| using f64 = double; | ||
|
|
||
| using isize = int64_t; |
There was a problem hiding this comment.
I'm wondering if ssize would be a better name (like std::ssize_t), but that's not terribly important.
Not worth going back and renaming everything for this.
6 participants
Proposal for common types that we will use to limit including more headers for these types.
I am open to changing the names I just went with Odin default typenames
Summary by CodeRabbit