fix dn_vector_custom_resize #102591
fix dn_vector_custom_resize #102591
Conversation
ghost
added
the
needs-area-label
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @tommcdon |
| DN_ASSERT (vector); | ||
|
|
||
| if (size == vector->_internal._capacity) | ||
| if (size == vector->size) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
ok，I have another fix, I will upload all later
am11
removed
the
needs-area-label
|
Hi @srxqds! Thanks for submitting this PR. Do you plan on addressing @lateralusX's feedback in the short term, or would it be OK to change this PR to draft mode while development is still in progress? |
|
Hi @srxqds! I'm changing the PR to a Draft PR - please let us know if you plan on working on it soon. |
5 participants
there is a bug when the size == capacity without set the vector->size