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

[move][stdlib] Add vector::split_off and vector::replace library methods #14752

Closed
wants to merge 2 commits into from

Conversation

igor-aptos
Copy link
Contributor

@igor-aptos igor-aptos commented Sep 25, 2024

Description

When we store un-drop-able objects in a vector, handling changes to vector needs to be done without dropping any elements in the process (or creating temporary ones).

Adding two missing utilities to help with that:

  • split_off - which moves part of the vector into a new vector
  • replace - which replaces value at particular index.

Both could have more efficient implementations, if implemented in rust. But seems like that is the case for many functions in vector.move, so starting with straightforward implementation.

How Has This Been Tested?

provided unit tests

Type of Change

  • New feature

Which Components or Systems Does This Change Impact?

  • Aptos Framework

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I identified and added all stakeholders and component owners affected by this change as reviewers
  • I tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

Copy link

trunk-io bot commented Sep 25, 2024

⏱️ 1h 11m total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
rust-move-unit-coverage 11m 🟩
rust-move-unit-coverage 10m 🟩
rust-move-tests 10m 🟥
rust-move-tests 10m 🟥
rust-move-tests 10m 🟥
rust-cargo-deny 6m 🟩🟩🟩
general-lints 5m 🟩🟩🟩
check-dynamic-deps 3m 🟩🟩🟩
rust-move-unit-coverage 2m 🟥
semgrep/ci 2m 🟩🟩🟩
file_change_determinator 34s 🟩🟩🟩
file_change_determinator 33s 🟩🟩🟩
permission-check 11s 🟩🟩🟩
permission-check 9s 🟩🟩🟩
permission-check 9s 🟩🟩🟩

settingsfeedbackdocs ⋅ learn more about trunk.io

Copy link

codecov bot commented Sep 25, 2024

Codecov Report

Attention: Patch coverage is 85.52632% with 11 lines in your changes missing coverage. Please review.

Please upload report for BASE (igor/move_mem_swap@514653c). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...party/move/move-vm/types/src/values/values_impl.rs 80.4% 8 Missing ⚠️
...ptos-move/framework/move-stdlib/src/natives/mem.rs 91.4% 3 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                  @@
##             igor/move_mem_swap   #14752   +/-   ##
=====================================================
  Coverage                      ?    59.8%           
=====================================================
  Files                         ?      854           
  Lines                         ?   208239           
  Branches                      ?        0           
=====================================================
  Hits                          ?   124599           
  Misses                        ?    83640           
  Partials                      ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@msmouse
Copy link
Contributor

msmouse commented Sep 25, 2024

in the summary: "remove - which replaces value at particular index." : "remove" should be "replace" 😂

@igor-aptos igor-aptos force-pushed the igor/vector_utilities branch 2 times, most recently from 6e1add6 to 9e1d6af Compare September 26, 2024 17:26
@igor-aptos igor-aptos changed the base branch from igor/native_compare to main October 3, 2024 20:17
@igor-aptos igor-aptos changed the base branch from main to igor/move_mem_swap October 3, 2024 20:19
/// Returns a newly allocated vector containing the elements in the range [at, len).
/// After the call, the original vector will be left containing the elements [0, at)
/// with its previous capacity unchanged.
public fun split_off<Element>(self: &mut vector<Element>, at: u64): vector<Element> {
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the diff with trim?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it's better to make trim native tho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, didn't realize, it is the same.

I think trim is a poor name for that - trim for a vector generally means make the capacity be equal to it's size.

Copy link
Contributor

Choose a reason for hiding this comment

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

the move vector seems not to have the concept of capacity...
idk who added this... but push_back one by one is not that efficient? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did that in #14862, let me just merge this into that PR.

@igor-aptos
Copy link
Contributor Author

merged into #14862

@igor-aptos igor-aptos closed this Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants