Skip to content

fix dn_vector_custom_resize #102591

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix dn_vector_custom_resize #102591

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/native/containers/dn-vector.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ dn_vector_custom_resize (
{
DN_ASSERT (vector);

if (size == vector->_internal._capacity)
if (size == vector->size)
Copy link
Member

@lateralusX lateralusX Jun 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this part is actually coming all the way from mono/mono GArray that have similar behavior, https://github.com/dotnet/runtime/blob/main/src/mono/mono/eglib/garray.c#L229.

Since this follow the std::vector::resize, https://en.cppreference.com/w/cpp/container/vector/resize, it should behave accordingly, so if new size == current size do nothing, new size smaller than current size, reduce elements, if new size is bigger than current size, it should grow to requested size and setup values to default.

if (size > vector->_internal._capacity) is an optimization to not call additional function in case not really needed (there is already capacity available in the vector), so that works, and the shrink scenario already checks against vector->size, so that is also fine.

Could you please add a test case for this scenario in https://github.com/dotnet/runtime/blob/main/src/mono/mono/eventpipe/test/dn-vector-tests.c#L945 as well? It should be possible to build and run these tests locally, enabling this when building Mono runtime, https://github.com/dotnet/runtime/blob/main/src/mono/mono/eventpipe/test/CMakeLists.txt, by setting ENABLE_EVENTPIPE_TEST, that should produce a test executable that runs all tests. NOTE, there might be tests broken in that suite, since it is currently run manual, and I guess not for sometime, but is normally straight forward to fix, please let me know if you have issues with the test suite.

Copy link
Member

@lateralusX lateralusX Jun 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is also worth pointing out that this function is currently not used in the dotnet runtime codebase. @srxqds do you use the library for standalone implementation or using it in Mono runtime embedding scenarios?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok，I have another fix, I will upload all later

return true;

if (size > vector->_internal._capacity)
Expand Down