Add UTF-16 Strings#34
Conversation
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds UTF-16 string infrastructure through new ChangesUTF-16 String Module
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant StringBuilder
participant Allocator
participant UTF16Buffer
Caller->>StringBuilder: Write(rune or String)
StringBuilder->>StringBuilder: GrowCapacity(required size)
StringBuilder->>Allocator: Reserve(new capacity)
Allocator-->>StringBuilder: allocated buffer
StringBuilder->>UTF16Buffer: copy encoded/appended UTF-16 units
StringBuilder-->>Caller: Error result and updated size
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 7
🤖 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/containers/string.cppm`:
- Around line 503-507: The WriteInt function has two issues: (1) negating
minimum signed integer values like INT_MIN causes undefined behavior, and (2)
GrowCapacity return values are not checked for failures, potentially leading to
out-of-bounds writes. In the signed value negation block (checking if val < 0),
use a safer approach such as converting to an unsigned type, using absolute
value safely, or checking for the minimum value before negating. Additionally,
wherever GrowCapacity is called within WriteInt, verify its return value
indicates success before continuing with the write operation; if allocation
fails, return an appropriate error code or handle the failure gracefully to
prevent buffer overruns.
- Around line 351-368: The `StringBuilder::Create` method has a stack-allocated
`fixed_allocator` variable that is used to initialize the `allocator` member,
but the allocator stores a pointer to this local variable which becomes invalid
after the function returns. To fix this, the `fixed_allocator` must be stored as
a member of the `StringBuilder` struct itself to ensure it persists for the
lifetime of the object, allowing subsequent growth and free operations to safely
access it without dereferencing dangling memory.
- Around line 437-446: The StringBuilder::Write methods call GrowCapacity with
only the incoming string length, not accounting for the existing buffer size.
Change the GrowCapacity calls from GrowCapacity(length) to GrowCapacity(size +
length) in both Write overloads (the utf16 version and the other overload
mentioned as also applying to lines 449-457) to ensure the total required
capacity is correctly calculated when appending to a non-empty builder.
- Around line 228-233: The Substr method incorrectly rejects valid substring
requests by returning EMPTY_STRING whenever the requested range touches or
exceeds the string boundary. Instead of returning EMPTY_STRING when pos + len >=
size, check only if pos >= size (completely out of bounds). When pos is valid
but the requested length extends beyond the string end, truncate len to size -
pos so the method returns the available substring. This truncation contract fix
will also resolve the false negatives in StartsWith and EndsWith methods that
depend on Substr.
- Around line 320-324: The `StringBuilder::Create` method has a unit mismatch
where the `capacity` parameter represents UTF-16 elements but is passed directly
to `allocator.alloc` as a byte size without conversion. Multiply `capacity` by
the size of a single UTF-16 element (sizeof(utf16)) when calling
`allocator.alloc` to correctly convert from element count to byte count. This
ensures the allocated storage is large enough to hold the requested number of
UTF-16 characters without under-allocating and risking buffer overflow.
- Around line 371-385: The Reserve method in StringBuilder has two issues: the
memcpy call copies size elements unconditionally even when newCapacity is
smaller, risking buffer overflow, and the oldDst.size field is set as an element
count rather than bytes, causing the allocator.free call to receive incorrect
size units. Fix the memcpy to copy only the minimum of size and newCapacity
elements (to avoid overflow), and set oldDst.size to capacity * sizeof(utf16)
instead of just capacity to ensure the allocator.free call receives the size in
bytes as expected.
- Around line 182-205: The StringCompare method has two critical bugs: the
min_size variable is initialized using itself as arguments to draco::math::min
instead of using the actual size members of both strings, which causes undefined
behavior; additionally, the null-pointer checks don't handle the case where both
string texts are null, causing two empty strings to incorrectly compare as
unequal. Fix the min_size initialization by comparing the size member of the
current string with the size member of the parameter string s, and add a check
at the beginning of the method to return 0 (equal) when both text pointers are
null, ensuring that EMPTY_STRING == EMPTY_STRING evaluates correctly.
🪄 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: f9a636d9-6f20-4bfc-a1a3-f770f0cfaa40
📒 Files selected for processing (6)
engine/native/core/CMakeLists.txtengine/native/core/containers/containers.cppmengine/native/core/containers/string.cppmengine/native/core/definitions/definitions.cppmengine/native/core/definitions/stdtypes.cppmengine/native/core/math/functions.cppm
|
The one structural thing that was made obvious to me when I was making my PR for allocators was that @OldDev78 wants as much implementation to be in .cpp files and there are some functions that are amenable to having their implementation in .cpp files. |
The constexpr items and templates need to be in the module, I'll see what @OldDev78 says about the .cpp implementation to see if it would make sense to put the very few StringBuilder functions into a .cpp file |
If a function serves best as template and/or can be effectively constrexpr (can be constant evaluated), then it's okay to be implemented in the .cppm. Otherwise, the implementation should just be separated into the .cpp. Take note that my biggest concern is to expose dependency on headers as little as possible (so, when I can, also move the includes to the .cpp). |
|
The integral concept is a merge conflict for me :/ |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
engine/native/core/containers/string.cppm (2)
349-354:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInfinite loop: base-2 loop condition checks
valinstead ofunsignedVal.The loop condition
while (val != 0)checks the original (unchanged) parameter, but the loop body only modifiesunsignedVal. Sincevalis never updated, this creates an infinite loop for any non-zero input when using base 2.The base-16 (line 356) and base-10 (line 361) cases correctly use
unsignedVal != 0.Suggested fix
} else if (base == 2) { - while (val != 0) { + while (unsignedVal != 0) { std::memcpy(buff + index, binaryNibbles[unsignedVal & 0xF], sizeof(binaryNibbles[0])); index += 4; unsignedVal >>= 4; }🤖 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/containers/string.cppm` around lines 349 - 354, The base-2 conversion loop condition checks the parameter `val` instead of `unsignedVal`, causing an infinite loop since `val` is never modified while only `unsignedVal` is updated in the loop body. Change the `while (val != 0)` condition in the `else if (base == 2)` block to `while (unsignedVal != 0)` to match the correct behavior used in the base-16 and base-10 cases.
194-197:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPotential integer overflow when sizes differ significantly.
The cast
(int)(size - s.size)can overflow when the size difference exceedsINT_MAX(e.g., comparing a 3-billion-element string against an empty one). The result would be incorrect comparison semantics.Suggested fix
if (result == 0) { - return (int)(size - s.size); + if (size == s.size) return 0; + return size < s.size ? -1 : 1; } - return result; + return result < 0 ? -1 : 1;🤖 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/containers/string.cppm` around lines 194 - 197, The cast (int)(size - s.size) in the return statement can overflow when the size difference exceeds INT_MAX, causing incorrect comparison results for strings with significantly different lengths. Fix this by safely comparing the sizes without casting a potentially large size_t difference to int. Instead, check if size is greater than s.size and return 1 if true, otherwise return -1 when sizes differ, to maintain proper comparison semantics without overflow risk.
🤖 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/containers/string.cppm`:
- Around line 32-49: The comparison operators (operator==, operator!=,
operator>, operator>=, operator<, operator<=) do not modify the String object
and should be marked as const member functions. Add the const keyword after the
parameter list for each of these six operators to indicate they don't modify the
object state and to allow them to be called on const String instances.
Additionally, update the StringCompare method declaration (referenced on line
31) to also be const since it is called by these operators and should not
require a mutable String instance.
---
Outside diff comments:
In `@engine/native/core/containers/string.cppm`:
- Around line 349-354: The base-2 conversion loop condition checks the parameter
`val` instead of `unsignedVal`, causing an infinite loop since `val` is never
modified while only `unsignedVal` is updated in the loop body. Change the `while
(val != 0)` condition in the `else if (base == 2)` block to `while (unsignedVal
!= 0)` to match the correct behavior used in the base-16 and base-10 cases.
- Around line 194-197: The cast (int)(size - s.size) in the return statement can
overflow when the size difference exceeds INT_MAX, causing incorrect comparison
results for strings with significantly different lengths. Fix this by safely
comparing the sizes without casting a potentially large size_t difference to
int. Instead, check if size is greater than s.size and return 1 if true,
otherwise return -1 when sizes differ, to maintain proper comparison semantics
without overflow risk.
🪄 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: 99d10637-fa46-476e-b1ad-5f8fdfb9ad32
📒 Files selected for processing (2)
engine/native/core/containers/string.cppengine/native/core/containers/string.cppm
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/cpp/runtime/core/containers/string.cpp`:
- Around line 79-82: The StringBuilder::Write method with the utf16 const *str
parameter calls StringLength(str) without first checking if the input pointer is
null, which will cause a null pointer dereference and crash. Add a null check at
the beginning of the StringBuilder::Write function (before calling StringLength)
and return an appropriate error code if str is null pointer, similar to how the
TODO comment suggests an assertion should be added.
- Line 96: In the string append operation at line 96 where memcpy is called to
copy str.text into the buffer, replace memcpy with memmove to safely handle the
case where str.text points into the builder's own buffer, preventing undefined
behavior and data corruption from overlapping memory regions.
- Around line 28-29: The capacity field is being assigned newDst.size which
represents byte count, but the write and grow logic throughout the code treats
capacity as the number of UTF-16 code units (elements). This mismatch causes a
critical overflow risk. Fix this by dividing newDst.size by sizeof(utf16) when
assigning to capacity at the line where capacity = newDst.size is set,
converting the byte size to the correct element count so that all subsequent
bounds checking and write operations use consistent units.
In `@Engine/cpp/runtime/core/containers/string.cppm`:
- Around line 358-363: The binary conversion loop in the base == 2 branch has a
non-terminating loop condition issue. The while loop checks `val != 0` but only
updates `unsignedVal` through right-shift operations, which means the loop
condition will never become false if val is non-zero, causing the loop to run
indefinitely and overflow the buffer. Change the loop condition from `while (val
!= 0)` to `while (unsignedVal != 0)` so that the loop terminates when
unsignedVal is completely processed to zero.
- Line 70: The StringIterator constructor performs pointer arithmetic on a
potentially null pointer when initializing the end member, which violates C++
standards. In the StringIterator constructor's member initializer list, add a
guard condition to check if s.text is null before performing the addition s.text
+ s.size. When s.text is nullptr, set end to nullptr instead of attempting the
pointer arithmetic, ensuring the constructor handles empty strings (like
EMPTY_STRING) safely without invoking undefined behavior.
- Around line 190-205: In the StringCompare function's runtime path (the else
branch following the consteval block), replace the memcmp call with
std::char_traits<char16_t>::compare to properly compare UTF-16 code units
instead of raw bytes, which will ensure correct ordering on all endianness
systems. Additionally, replace the cast from isize to int in the final return
statement that compares size and s.size with a ternary operator that returns -1
if size is less than s.size, 1 if greater, or 0 if equal, to avoid integer
overflow and maintain correct relational semantics for large size deltas.
🪄 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: 785a91cc-508b-4684-bbb1-58fa235dcf5a
📒 Files selected for processing (7)
Engine/cpp/runtime/core/CMakeLists.txtEngine/cpp/runtime/core/containers/containers.cppmEngine/cpp/runtime/core/containers/string.cppEngine/cpp/runtime/core/containers/string.cppmEngine/cpp/runtime/core/definitions/definitions.cppmEngine/cpp/runtime/core/definitions/stdtypes.cppmEngine/cpp/runtime/core/math/functions.cppm
✅ Files skipped from review due to trivial changes (1)
- Engine/cpp/runtime/core/containers/containers.cppm
|
Needs a rebase + should be updated |
JoltedJon
force-pushed
the
strings
branch
3 times, most recently
from
05a5a71 to
f7e9316
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
Engine/cpp/Runtime/Core/Containers/String.cpp (2)
92-100: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse
memmoveinstead ofmemcpyinWrite(String).If
str.textpoints into the builder's own buffer (e.g., afterReset()), the source and destination regions overlap, makingmemcpyundefined behavior.memmoveis the safe primitive. Previously flagged, still unfixed.🛡️ Proposed fix
- memcpy(buffer + size, str.text, str.size * sizeof(utf16)); + memmove(buffer + size, str.text, str.size * sizeof(utf16));🤖 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/cpp/Runtime/Core/Containers/String.cpp` around lines 92 - 100, Update StringBuilder::Write(String) to use memmove rather than memcpy when copying str.text into buffer, preserving the existing byte count and size update so overlapping source and destination regions are handled safely.
80-90: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNull dereference in
Write(utf16 const*)— still unaddressed.
StringLength(str)is called without checking for null. A null input crashes before returning an error. This was flagged in a previous review and remains unfixed.🛡️ Proposed fix
StringBuilder::Error StringBuilder::Write(utf16 const *str) { + if (str == nullptr) { + return Error::IllegalAddressRange; + } // TODO(Jon) assert not null isize length = StringLength(str);🤖 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/cpp/Runtime/Core/Containers/String.cpp` around lines 80 - 90, Update StringBuilder::Write(utf16 const *str) to validate str before calling StringLength or memcpy; when it is null, return the established appropriate Error value without modifying the builder, while preserving the existing behavior for valid strings.Engine/cpp/Runtime/Core/Containers/String.cppm (1)
351-356: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winCritical:
WriteIntbase-2 loop still has non-terminating condition.Line 352 checks
valbut onlyunsignedValis modified inside the loop. For any non-zero input, the loop never terminates and overflowsbuff. This was flagged in a previous review and marked as addressed, but the bug persists in the current code — it may have been lost during the rebase.🐛 Proposed fix
} else if (base == 2) { - while (val != 0) { + while (unsignedVal != 0) { std::memcpy(buff + index, binaryNibbles[unsignedVal & 0xF], sizeof(binaryNibbles[0])); index += 4; unsignedVal >>= 4; }🤖 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/cpp/Runtime/Core/Containers/String.cppm` around lines 351 - 356, Update the base-2 branch of WriteInt so its loop condition checks the value being shifted, unsignedVal, rather than the unchanged val. Preserve the existing nibble-copy and index-advance behavior while ensuring the loop terminates when unsignedVal reaches zero.
🧹 Nitpick comments (1)
Engine/cpp/Runtime/Core/Math/Functions.cppm (1)
13-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding
noexcepttominandmax.These functions only compare arithmetic types, which cannot throw. Other simple functions in this file (
sqr,abs,sign) are markednoexcept. Adding it here maintains consistency and enables additional compiler optimizations.♻️ Proposed refactor
- template <arithmetic T> - constexpr T min(T a, T b) { + template <arithmetic T> + constexpr T min(T a, T b) noexcept { return a < b ? a : b; } - template <arithmetic T> - constexpr T max(T a, T b) { + template <arithmetic T> + constexpr T max(T a, T b) noexcept { return b < a ? a : b; }🤖 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/cpp/Runtime/Core/Math/Functions.cppm` around lines 13 - 21, Mark the arithmetic min and max function templates as noexcept, matching the existing noexcept specification of nearby simple functions such as sqr, abs, and sign.
🤖 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/cpp/Runtime/Core/CMakeLists.txt`:
- Line 12: Update the target_link_libraries declaration for Containers to
reference the correctly cased Definitions target created by
add_modules_library(Definitions), replacing the lowercase definitions entry
while preserving the other linked targets.
In `@Engine/cpp/Runtime/Core/Containers/String.cpp`:
- Around line 9-33: Guard the memcpy in StringBuilder::Reserve so it runs only
when size is nonzero (and buffer is valid), avoiding a null source pointer on
the first allocation. Preserve the existing allocation, capacity update, and
zero-size behavior.
---
Duplicate comments:
In `@Engine/cpp/Runtime/Core/Containers/String.cpp`:
- Around line 92-100: Update StringBuilder::Write(String) to use memmove rather
than memcpy when copying str.text into buffer, preserving the existing byte
count and size update so overlapping source and destination regions are handled
safely.
- Around line 80-90: Update StringBuilder::Write(utf16 const *str) to validate
str before calling StringLength or memcpy; when it is null, return the
established appropriate Error value without modifying the builder, while
preserving the existing behavior for valid strings.
In `@Engine/cpp/Runtime/Core/Containers/String.cppm`:
- Around line 351-356: Update the base-2 branch of WriteInt so its loop
condition checks the value being shifted, unsignedVal, rather than the unchanged
val. Preserve the existing nibble-copy and index-advance behavior while ensuring
the loop terminates when unsignedVal reaches zero.
---
Nitpick comments:
In `@Engine/cpp/Runtime/Core/Math/Functions.cppm`:
- Around line 13-21: Mark the arithmetic min and max function templates as
noexcept, matching the existing noexcept specification of nearby simple
functions such as sqr, abs, and sign.
🪄 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: f164da8d-2fcf-46b6-b6e0-63d752119c64
📒 Files selected for processing (7)
Engine/cpp/Runtime/Core/CMakeLists.txtEngine/cpp/Runtime/Core/Containers/Containers.cppmEngine/cpp/Runtime/Core/Containers/String.cppEngine/cpp/Runtime/Core/Containers/String.cppmEngine/cpp/Runtime/Core/Definitions/Definitions.cppmEngine/cpp/Runtime/Core/Definitions/StdTypes.cppmEngine/cpp/Runtime/Core/Math/Functions.cppm
4 participants
Add UTF-16 Strings
Summary by CodeRabbit