Skip to content
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

Potential bug in u64_array_remove_duplicates #376

Open
frwdrik opened this issue Dec 20, 2024 · 1 comment
Open

Potential bug in u64_array_remove_duplicates #376

frwdrik opened this issue Dec 20, 2024 · 1 comment
Labels
Codebase Pertains to the core codebase layers, used in both the debugger and linker.

Comments

@frwdrik
Copy link

frwdrik commented Dec 20, 2024

I think there's a bug in u64_array_remove_duplicates. When input is an array of size N > 0 consisting of a single value x, repeated N times, it doesn't return what I expect: Instead of returning an array { x } of count one, it returns an empty array.

Small reproducible test case:

int main() {
  Arena *arena = arena_alloc();

  U64Array arr = {};
  arr.count = 2;
  arr.v = push_array_no_zero(arena, U64, arr.count);
  arr.v[0] = 1;
  arr.v[1] = 1;
  
  U64Array uniq = u64_array_remove_duplicates(arena, arr);
  AssertAlways(uniq.count == 1); // This (surprisingly) traps

  return 0;
}

Compiling with GCC 14.2.1 and running on Linux 6.12.4, this causes a trap from the AssertAlways. The expected behaviour would be that the assert passes, like in this modified version:

int main() {
  Arena *arena = arena_alloc();

  U64Array arr = {};
  arr.count = 3;
  arr.v = push_array_no_zero(arena, U64, arr.count);
  arr.v[0] = 1;
  arr.v[1] = 1;
  arr.v[2] = 2;
  
  U64Array uniq = u64_array_remove_duplicates(arena, arr);
  AssertAlways(uniq.count == 2); // This doesn't trap
  return 0;
}

Note that by adding a different element 2, the assert now passes. In this case the different element was inserted at the end of the array, but that is (as one would expect) not significant.

I came across this while studying the various functions in the base layer of this project. The overall style and design of its building blocks are a great inspiration. Thanks for making this project's source public.

If you confirm this is a bug, I can have a try at a PR to fix it.

@Mallchad
Copy link

This API is specific to the linker it seems. and it very clearly removes contiguous duplicates, not making a single instance in the array.
B32 is_unique = in.v[i - 1] != in.v[i];
It's just not obvious this is unintended, but hey, it would be nice if it had a short docstring.

@ryanfleury ryanfleury added the Codebase Pertains to the core codebase layers, used in both the debugger and linker. label Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Codebase Pertains to the core codebase layers, used in both the debugger and linker.
Projects
None yet
Development

No branches or pull requests

3 participants