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

Add semantics to move a blob. #154

Closed
wants to merge 2 commits into from
Closed

Conversation

szmyd
Copy link
Collaborator

@szmyd szmyd commented Aug 2, 2023

We don't need to wrap a byte_array_impl (or other blob) in a shared_ptr/unique_ptr if we have move semantics since the underlying is just a pointer mostly anyways as well. The user-defined destructor will handle RAII, no need for a pointer to a pointer I think?

We can enforce ownership via the API signature, (e.g. void put(Blob&& blob, ... forcing the user to release ownership and making for cleaner code with less de-referencing and checking for nullptr.

@szmyd szmyd force-pushed the add_blob_move branch 3 times, most recently from 78b5807 to 4da802f Compare August 2, 2023 01:05
@codecov-commenter
Copy link

codecov-commenter commented Aug 2, 2023

Codecov Report

Merging #154 (fd7c0b0) into master (6d6efcf) will decrease coverage by 0.54%.
The diff coverage is 53.33%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #154      +/-   ##
==========================================
- Coverage   64.02%   63.49%   -0.54%     
==========================================
  Files          71       71              
  Lines        4306     4303       -3     
  Branches      530      529       -1     
==========================================
- Hits         2757     2732      -25     
- Misses       1323     1343      +20     
- Partials      226      228       +2     
Components Coverage Δ
AuthManager 94.40% <ø> (ø)
Cache 25.80% <ø> (-4.15%) ⬇️
FDS 68.24% <53.33%> (-0.09%) ⬇️
FileWatcher 55.90% <ø> (ø)
Flip 65.67% <ø> (ø)
gRPC 77.10% <ø> (ø)
Logging 29.41% <ø> (ø)
Metrics 80.20% <ø> (ø)
Options 100.00% <ø> (ø)
Setting 56.68% <ø> (ø)
StatusObject 73.83% <ø> (ø)
Utility 83.49% <ø> (ø)
Version 95.83% <ø> (ø)


blob() : blob{nullptr, 0} {}
blob(uint8_t* const b, const uint32_t s) : bytes{b}, size{s} {}
std::byte* bytes{nullptr};
Copy link
Contributor

Choose a reason for hiding this comment

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

There are several places that are using blob as uint8_t*. Shouldn't this be a major version upgrade?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

probably, though are we using master with the sisl::blob anywhere? I don't think OM uses it and 1.3 AM is still on stable right? But you're right, a 10.x is warranted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we are only using it in the 2.0 data side of things.

Copy link
Contributor

@raakella1 raakella1 left a comment

Choose a reason for hiding this comment

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

lgtm! I will wait for @hkadayam 's review before approving.

@yamingk
Copy link
Contributor

yamingk commented Aug 2, 2023

One use case of the shared-ptr of byte-arrray in HomeStore is component-A passes the shared point of byte-array to Component-B, and B read that memory (for write to disk), and then B finishes the API (it is a sync-call) and return to A and A still owns that byte-array as A will periodically update that same memory and pass it to B by calling that API.

@szmyd szmyd closed this Aug 3, 2023
@szmyd szmyd deleted the add_blob_move branch September 20, 2023 18:52
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.

4 participants