Repository navigation
Add std::multimap constructors and overloaded insert - #66
Janeirungu01 wants to merge 4 commits into
Conversation
teor2345
left a comment
There was a problem hiding this comment.
Just something I noticed when checking your code before running CI, @thunderseethe will do most of the reviews here 🙂
|
noted, I am going to move the comments |
| pub fn new(a: (K, V), b: (K, V), c: (K, V)) -> StdMultimap { | ||
| StdMultimap::from_initializer_list_3(a, b, c) | ||
| } | ||
| } |
There was a problem hiding this comment.
This is a good start. What would it take to cover the multimap constructors that take a Compare argument?
There was a problem hiding this comment.
The Compare constructors allow std::multimap to use a comparator that defines how keys are ordered. By default, the current implementation uses std::less<int>, which sorts keys in ascending order. To cover a different comparator, a wrapper for std::multimap<int, int, std::greater<int>> can be introduced tests added to verify that iteration returns keys in descending order.
Depending on the constructors to cover, they may accept a comparator only, a comparator with an allocator or iterator ranges together with a comparator.
Since the current wrapper uses a fixed C++ type, std::multimap<int, int, std::less<int>>, supporting alternative comparators would require separate wrapper types or a redesign that allows the underlying C++ container type to vary. We would then need to implement and test the selected constructor overloads for each supported comparator.
I haven't implemented them because the current wrapper uses a fixed type.
| /// `map.erase(&pos)` from compiling, because `erase` needs `&mut map` while the iterator | ||
| /// still borrows the map. | ||
| #[cfg_attr(not(test), expect(dead_code, reason = "Only used in tests"))] | ||
| pub struct StdMultimapIter(*mut c_void, PhantomData<(K, V)>); |
There was a problem hiding this comment.
What would it take if I wanted to drive this iterator from C++?
There was a problem hiding this comment.
To drive this iterator from C++, we need to add C++ helpers to create the iterator, advance it, compare it and read the current key value pair. These helpers could be called from Rust through the existing cpp! interface.
Currently, StdMultimapIter stores an opaque pointer to the C++ iterator representation. The existing implementation creates an iterator using lower_bound and passes it to erase, but it doesn't provide operations for traversing the map.
The main challenge is managing iterator's lifetime. Since the current wrapper does not tie the iterator's map to lifetime, I would need to ensure that the map is never destroyed and that the iterator is not used after its referenced elemens have been erased.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
What would it take to prove an interface for find/count/contains and which of these make sense to provide FFI interfaces for and which do not?
There was a problem hiding this comment.
count is already wrapped as the helper count_key, it returns the number of elements with a specified key. It is convenient to expose using FFI since it returns a numeric value and doesn't require an iterator.
contains and find would follow the same pattern, with find returning StdMultimapIter similar to lower_bound. I would wrap them as non-overloaded helpers.
contains returns whether the key exists. It is convenient to expose as a boolean, but std::multimap only provides contains() in C++20 and later.
find returns an iterator to an element or end() if key does not exist. Exposing it through FFI is possible, but it requires safe iterator handling and lifetime management. A convenient way to expose is as a boolean to check whether the key exists.
I can add find and contains as helpers if required.
This PR adds an initial Rust wrapper for std::multimap in cpp-overload-test/src/stdcpp/multimap.rs that adds four methods -
newinsert,erase,merge. Overloads excluded by the task guidelines are documented in the source file.1. Constructor overloads: new
These four signatures differ by arguement count and argument types. The slice-based and single-entry constructors provide Rust-friendly interfaces to the underlying C++ container.
2. Insertion overloads: insert
The hinted insertion method adapts the C++ iterator-hint interface to a Rust-facing key-based interface. The slice-based method provides an alternative to C++ range and initializer-list insertion.
The key-range overload adapts the C++ iterator-range interface by finding the range boundaries with lower_bound. The iterator overload removes a single element, even when duplicate keys exist.
merge(&mut self, other: &mut StdMultimap) - moves elements from another multimap into the destination map.
This wraps the C++ merge(multimap&) overload for maps using the same comparator type.
3. Tests and Documentation
Added tests for default, move, slice-based, and fixed-size pair construction.
Added tests for ordinary, hinted, and slice-based insertion.
Added tests for key-based, key-range, and iterator-based erasure, including duplicate keys and an end() iterator.
Added a test for merging two multimaps.
Documented compatibility limitations, unsupported overloads, and Rust API ergonomics.
4. Scope
The current scope currently covers four overloaded method names:
new,insert,eraseandmerge.AI usage: I have used Claude to debug from_slice and insert_slice_internal overloads.
I have written the code myself and most concepts have been borrowed from forward_list
partially adresses #40
Limitationsdoc sectionErgonomicsdoc sectionIncompatibilitiesdoc sectionWorkaroundsdoc sectionInexpressible Overloadsdoc section