Skip to content

Conversation

@frol
Copy link

@frol frol commented Jul 31, 2015

std::unordered_map<std::string, int, std::hash<size_t> >

Things like above cannot be packed without this patch.

@redboltz
Copy link
Contributor

redboltz commented Aug 2, 2015

Thank you for sending the PR. It looks good to me. After adding some tests, I will merge it.

@redboltz
Copy link
Contributor

redboltz commented Aug 2, 2015

BTW, it seems that other C++03 containers' adapters such as std::vector should have allocator tempalte parameter, is that right?

@frol
Copy link
Author

frol commented Aug 2, 2015

@redboltz Yes, it seems so. I fixed only CPP11 because we use those in our project. I've never used allocator in std::vector, so I haven't noticed lack of it in msgpack-c. Thank you for this nice msgpack-c project!

@redboltz
Copy link
Contributor

redboltz commented Aug 3, 2015

Yes, it seems so. I fixed only CPP11 because we use those in our project. I've never used allocator in std::vector, so I haven't noticed lack of it in msgpack-c.

@frol, I just started adding all containers' extra template parameter support. During that process, I realized that It is better that defining each template parameter instead of using variadic template parameters.

For example,

template <typename K, typename V, typename... OtherTypes>

is changed to

template <typename K, typename V, typename Hash, typename Pred, typename Alloc>

Because it describes code intention more, also it can keep consistency with C++03 container supporting code that I am adding. e.g.) std::map.

So, I will modify your PR a little.

Thank you for this nice msgpack-c project!

I am very glad to hear that :)

@frol
Copy link
Author

frol commented Aug 3, 2015

So, I will modify your PR a little.

Actually, I started with explicit template parameters, but later I found usage of variadic parameters in your code and decided to use those. I also prefer explicit over implicit, so feel free to change this.

BTW, I don't really see a point in modifying my PR as it will be completely changed, so you may close it and make those changes on master.

@redboltz redboltz merged commit f986370 into msgpack:master Aug 3, 2015
@redboltz
Copy link
Contributor

redboltz commented Aug 3, 2015

I've just merged. It contains all existing containers extra parameters support.

@frol
Copy link
Author

frol commented Aug 3, 2015

Awesome! I will test it on our project today.

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