Skip to content

Conversation

@dfaust
Copy link
Contributor

@dfaust dfaust commented Mar 26, 2017

Add a multimap! macro plus some minor fixes.

Copy link
Member

@thejpster thejpster left a comment

Choose a reason for hiding this comment

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

Thank you so much for this. I'm so happy someone else found this crate useful enough to send a PR!

Could you briefly explain the macro - I can't quite work out what's going on, especially as it's recursive. Could you also change the test to verify the macro had actually created the multimap you expect it to. I wonder if we should implement PartialEq - it might make those macro tests easier as we could check for equality with a hand built multimap.

Thanks!

@dfaust
Copy link
Contributor Author

dfaust commented Mar 26, 2017

The macro is pretty much copied from https://github.com/bluss/maplit, so to be honest I'm not 100% sure what it does myself :). Unfortunately I couldn't find anything about calculating the length of the macro arguments. The recursion is due to the macro being re-used to perform multiple calculation steps using the prefixes @single and @count.
The last two arms are the entry points. The first has a trailing comma, the second doesn't. In case of a trailing comma it is stripped away and the macro is called again.
At first the length of the arguments is calculated by calling the macro with the @count prefix.
The length is then used to allocate enough memory.
After that all arguments are inserted into the map.

Could you also change the test to verify the macro had actually created the multimap you expect it to

sure

Implementing PartialEq would make sense Debug probably as well (needed for assert!).

@thejpster
Copy link
Member

Right, gotcha. Thanks.

@dfaust dfaust force-pushed the master branch 2 times, most recently from 434088a to 6e5c781 Compare March 26, 2017 16:57
@thejpster thejpster merged commit c631b75 into rust-embedded-community:master Mar 26, 2017
@thejpster
Copy link
Member

Superb.

@thejpster
Copy link
Member

Submitted 1.1.0.

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.

2 participants