-
-
Notifications
You must be signed in to change notification settings - Fork 291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a test case for List.dropAt at index in the middle of list #7071
Add a test case for List.dropAt at index in the middle of list #7071
Conversation
@ageron will you investigate these test failures? I am guess you added this test as you suspected there was a bug in this implementation. Just wanting to clarify what you are thinking here. |
This should be the fix I think: diff --git a/crates/compiler/builtins/bitcode/src/list.zig b/crates/compiler/builtins/bitcode/src/list.zig
index e89b39c561..c95e6e8063 100644
--- a/crates/compiler/builtins/bitcode/src/list.zig
+++ b/crates/compiler/builtins/bitcode/src/list.zig
@@ -618,6 +618,16 @@ pub fn listDropAt(
) callconv(.C) RocList {
const size = list.len();
const size_u64 = @as(u64, @intCast(size));
+
+ // NOTE
+ // we need to return an empty list explicitly,
+ // because we rely on the pointer field being null if the list is empty
+ // which also requires duplicating the utils.decref call to spend the RC token
+ if (size <= 1) {
+ list.decref(alignment, element_width, elements_refcounted, dec);
+ return RocList.empty();
+ }
+
// If droping the first or last element, return a seamless slice.
// For simplicity, do this by calling listSublist.
// In the future, we can test if it is faster to manually inline the important parts here.
@@ -638,25 +648,16 @@ pub fn listDropAt(
// were >= than `size`, and we know `size` fits in usize.
const drop_index: usize = @intCast(drop_index_u64);
- if (elements_refcounted) {
- const element = source_ptr + drop_index * element_width;
- dec(element);
- }
-
- // NOTE
- // we need to return an empty list explicitly,
- // because we rely on the pointer field being null if the list is empty
- // which also requires duplicating the utils.decref call to spend the RC token
- if (size < 2) {
- list.decref(alignment, element_width, elements_refcounted, dec);
- return RocList.empty();
- }
-
if (list.isUnique()) {
- var i = drop_index;
- const copy_target = source_ptr;
+ if (elements_refcounted) {
+ const element = source_ptr + drop_index * element_width;
+ dec(element);
+ }
+
+ const copy_target = source_ptr + (drop_index * element_width);
const copy_source = copy_target + element_width;
- std.mem.copyForwards(u8, copy_target[i..size], copy_source[i..size]);
+ const copy_size = (size - drop_index - 1) * element_width;
+ std.mem.copyForwards(u8, copy_target[0..copy_size], copy_source[0..copy_size]);
var new_list = list;
|
I initially wanted to fix the issue, but I started digging into the Zig code and realized I was in over my head, so I preferred to explain what I found and add a test case, and hope that someone smarter than me can fix the actual issue. |
…o add-list-dropat-test-case
Hi @bhansconnect , thanks for the fix, I just tried it and it works. 👍 I misunderstood your comment above, I thought you had pushed the fix in another PR, but I think you meant for me to add it here, so I just did. Sorry for the confusion! |
Yeah, I probably should have just pushed a PR, but I wasn't sure if someone was already working on a fix or not. I just thought I knew the what the fix would be so I wrote something up and posted, but didn't want to stomp someone else's work if this was already being fixed. |
@bhansconnect , please feel to approve this PR or to submit another one, as you prefer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 😄
No description provided.