Repository navigation
Conversation
teor2345
left a comment
There was a problem hiding this comment.
Today I updated the PR template to make it clear that all overloads need to be covered, so I updated this PR's description to match it. I also fixed the GitHub checklist formatting.
For this class constructor, there are many overloads, so we can just wrap the ones that are more likely to conflict.
No need to add any more overloaded methods, the constructor and the insert method have enough overloads.
| StdUnorderedMultiset::from_slice(items) | ||
| } | ||
|
|
||
| pub fn new(a: T, b: T) -> StdUnorderedMultiset { |
There was a problem hiding this comment.
To check that an overload set works in Rust, we need to wrap all of the C++ constructor overloads.
There are 14 constructor overloads in C++, excluding overloads that only differ by an allocator argument:
https://en.cppreference.com/cpp/container/unordered_multiset/unordered_multiset
That's a lot of overloads, so you don't have to implement them all. Instead, pick overloads that are very different from each other. For example, you could add one overload starting with these arguments:
- bucket_count
- unordered_multiset
- from_range_t
It would also be interesting to use C++ default arguments to call an overload with just one argument of type:
- hash
- key_equal
This would be enough to cover the constructor overload set reasonably well.
Please document the lack of default argument values in Rust as a limitation.
| //! | ||
| //! - Type annotations are currently required for some overloads where inference should work. | ||
| //! - The `as_slice()` call is required to match the slice overload, as arrays don't automatically coerce. | ||
| //! - Initializer list overloads require separate functions for each argument count, which is not ergonomic. |
There was a problem hiding this comment.
Please split up this documentation, and move it to the specific types or methods with these problems. That way, when we fix a problem, we are more likely to notice the documentation and update it.
| overload! { | ||
| impl StdUnorderedMultiset { | ||
| /// Insert a single value into the set. | ||
| pub fn insert(&mut self, value: T) { |
There was a problem hiding this comment.
There's a lot of insert overloads as well, feel free to update this PR in stages, and ask for a review after every stage.
Ok, I will sync my forked repo and fix all necessary update you mentioned. Thank you for the feedback |
Added a wrapper for std::unordered_multiset with 5 overloads (constructors and insert).
Limitationsdoc sectionErgonomicsdoc sectionIncompatibilitiesdoc sectionWorkaroundsdoc sectionInexpressible Overloadsdoc sectionMark items in this checklist as done by clicking the box next to each item.
Focus on Interesting Overloads
Only wrap functions that have overloads. (It is ok to wrap non-overloaded functions just for testing.)
Ignore these overloads:
noexcept: not an interesting differenceswap: no equivalent in Rust (which uses memcpy for assignment/swaps)Only code one example of overloads that:
const/non-const,&/&&(or&/&mut)before_begin/begin,end)You can look at stdcpp/forward_list.rs (working overloads) and stdcpp-forward-list.rs (failing overloads) for example code.
Closes #41