Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add BoundedBTreeSet #8750

Merged
3 commits merged into from
May 10, 2021
Merged

Add BoundedBTreeSet #8750

3 commits merged into from
May 10, 2021

Conversation

coriolinus
Copy link
Contributor

Part of #8719

@coriolinus coriolinus added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels May 6, 2021
@coriolinus coriolinus self-assigned this May 6, 2021
@coriolinus coriolinus mentioned this pull request May 6, 2021
20 tasks
@bkchr
Copy link
Member

bkchr commented May 6, 2021

IMHO we have now reached the point where this should be abstracted. IMHO I don't see any point in copying this for every type in the std library (I know that we don't need every type). However this is mainly code copying. Which should be doable with some generic container or a macro that generates the stuff.

@coriolinus
Copy link
Contributor Author

I'm not sure how well it can be automated. The desired abstraction involves

  • Deref / AsRef impls for appropriate types
  • passthrough methods for &mut self methods which cannot increase the size
  • try_X methods for &mut self methods which can increase the size

Knowing which types are appropriate, and which methods can increase the size, seems to require human intervention.

That said, once this is in, we'll have bounded variants for lists, sets, and maps. Those should suffice for all reasonable storage cases. I'm not anticipating adding more PRs like this soon.


/// Remove an item from the set, returning whether it was previously in the set.
///
/// The item may be any borrowed form of the set's item type, but the ordering on the borrowed
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL 👍

}
}

impl<T, S> From<BoundedBTreeSet<T, S>> for BTreeSet<T>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might miss this in BoundedVec and BoundedBTreeMap, good one!

@kianenigma
Copy link
Contributor

kianenigma commented May 7, 2021

IMHO we have now reached the point where this should be abstracted. IMHO I don't see any point in copying this for every type in the std library (I know that we don't need every type). However this is mainly code copying. Which should be doable with some generic container or a macro that generates the stuff.

I don't have as strong opinion about BtreeMap and Set. On one hand they are types that we support in storage and we are a library, on the other hand we don't really use them (for good reasons) and we shouldn't overkill it. Either way I am happy with this PR.

About your comment on abstraction: I think what we do need is a SortedVec next to our BoundedVec, because we have quite a few storage items that have the assumption that they must always be sorted. And implementation-wise I was thinking of something like this:

struct AttributeVec<T, Attr: Attribute>(Vec<T>);

/// An attribute to a container like a Vec
trait Attribute {
  fn pre_insert();
  fn post_insert();
  fn pre_delete(); 
  fn post_delete(); 
  // and some other hooks/utilities that allow an attribute to control how a container mutates.
}

/// An attribute that ensures a container's length never goes beyond a limit.
struct Bounded;
impl Attribute for Bounded {
  fn pre_insert() { /* ensure bound */}
}

/// An attribute that ensures a container is always sorted. 
struct Sorted;
impl Attribute for Sorted {
  fn post_insert() { /* do sort! */}
  fn post_delete() { /* do sort! */}
}

// and we can have other attributes, and attributes can be combined by aggregating them into a tuple. 
#[impl_trait_for_tuples(18)]
if Attribute for Tuple { /* ... */ }

/// 
type BoundedVec<T> = AttributeVec<T, Bounded>;
type SortedVec<T> = AttributeVec<T, Sorted>;
type BoundedSortedVec<T> = AttributeVec<T, (Bounded, Sorted)>;

But to be honest there are a lot of empty pieces in my brain dump above, and I am worried that this would add so much complexity that we would regret it and just go back to the simpler way of copy-pasta.

@cla-bot-2021
Copy link

cla-bot-2021 bot commented May 7, 2021

@kianenigma No commands could be detected from your comment.

Co-authored-by: Kian Paimani <[email protected]>
@joao-paulo-parity
Copy link
Contributor

Hello?

@kianenigma
Copy link
Contributor

do you love me cla bot?

@coriolinus
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented May 10, 2021

Trying merge.

@ghost ghost merged commit 92a7a12 into master May 10, 2021
@ghost ghost deleted the prgn-bounded-btree-set branch May 10, 2021 07:49
@jakoblell jakoblell added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels May 31, 2021
nazar-pc pushed a commit to autonomys/substrate that referenced this pull request Aug 8, 2021
* Add `BoundedBTreeSet`

Part of paritytech#8719

* fix copy-pasta errors

Co-authored-by: Kian Paimani <[email protected]>

Co-authored-by: Kian Paimani <[email protected]>
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants