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

chore: merge BoundedStorable into Storable #94

Merged
merged 17 commits into from
Aug 17, 2023

Conversation

ielashi
Copy link
Contributor

@ielashi ielashi commented Jun 23, 2023

Problem

We'd like to expand the functionality of BTreeMap to support unbounded types. To add this support in a backward-compatible way, the BTreeMap would need to support both bounded types (i.e. types that implement BoundedStorable) and unbounded types (i.e. types that don't implement BoundedStorable).

Rust doesn't support generic specializations, so we cannot have the same BTreeMap support both use-cases simultaneously. It can either support only BoundedStorable types, or all types implementing Storable, but without the knowledge of whether a BoundedStorable implementation is available for those types.

Solution

Merge BoundedStorable and Storable into the same trait. The one downside of this approach is that checking whether or not a bound exists now happens at run-time.

This impacts the developer experience. Prior to the PR, if you try to store a String inside a StableVec, the compiler will complain that String doesn’t implement BoundedStorable. But, with this solution, it’ll happily compile, and only when you run your code will you see a panic that String is not bounded.

This PR relates to #84 and #69.

@ielashi ielashi marked this pull request as ready for review June 23, 2023 11:25
@ielashi ielashi requested review from a team and roman-kashitsyn as code owners June 23, 2023 11:25
@ielashi ielashi changed the title chore: Merge BoundedStorable into Storable chore: merge BoundedStorable into Storable Jun 23, 2023
src/storable.rs Outdated Show resolved Hide resolved
@dragoljub-duric
Copy link
Contributor

dragoljub-duric commented Jun 23, 2023

I am more in favor of the second approach with the separate UnboundedBTreeMap:

  1. The first approach with the check in the run-time in my opinion does not go in line with what most libraries in the Rust offer. I am afraid that we can shoot ourselves in the leg with that approach.
  2. When the user uses UnboundedBTreeMap for the type that implements BoundedStorable it is his mistake, and he will pay the price for it. I see it more or less as the difference between map and unordered_map (in programming languages) because you can do everything with the map as you can do with the unordered_map with some additional things, but if you do not need those additional things you may waste the resources using map. Hence that sounds like a reasonable tradeoff for me.

@ielashi
Copy link
Contributor Author

ielashi commented Jun 27, 2023

  1. The first approach with the check in the run-time in my opinion does not go in line with what most libraries in the Rust offer. I am afraid that we can shoot ourselves in the leg with that approach.

What is an example of these issues that you're thinking may arise?

@ielashi
Copy link
Contributor Author

ielashi commented Jul 19, 2023

The community has voted for this solution on the forum. I think this PR is now ready to be reviewed/merged.

@ielashi ielashi enabled auto-merge (squash) August 17, 2023 11:37
@ielashi ielashi disabled auto-merge August 17, 2023 11:39
@ielashi ielashi merged commit d56fba1 into main Aug 17, 2023
3 checks passed
@ielashi ielashi deleted the ielashi/remove_bounded_storable branch August 17, 2023 11:45
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