Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use template instead of unordered_map in imap::put_all #1218

Open
ldrozdz93 opened this issue Jan 12, 2024 · 6 comments
Open

Use template instead of unordered_map in imap::put_all #1218

ldrozdz93 opened this issue Jan 12, 2024 · 6 comments

Comments

@ldrozdz93
Copy link

Hi,

The member function hazelcast::client::imap::put_all is constrained to use a std::underdered_map as the input values. The user might want to use a more cache-friendly alternative, like boost::unordered_flat_map or std::flat_map, or maybe a std::vector<std::pair<...>> for some specific reasons.

Question:
Could I add such template put_all support? I could implement that and submit a PR, but I need any confirmation from anybody responsible for this repo, that it would actually make sense and there are people here that would review and be interested in merging it, so that my time is not actually wasted :)

Best regards,
Łukasz

@ihsandemir
Copy link
Collaborator

Hello @ldrozdz93 Yes, i think it makes sense. You are going to generalize the usage which would also cover the existing API put_all. The only point to make sure is that we do not break the existing user APIs. So, any existing user should be able to continue using the same API without any need for the code change and existing codes compile without any problems.

Please go ahead and submit the PR. Thanks for the contribution.

@ldrozdz93
Copy link
Author

One detail before I proceed:

make sure is that we do not break the existing user APIs

Just to make sure what we mean by that exactly. Imagine the following class:

struct MyVec : std::vector<std::pair<int, int>> {
    operator const std::unordered_map<int, int>&();
    ....
};
...
map->put_all(MyVec{});

Should the template put_all still convert this to an unordered_map (full backwards compatibility), or should it iterate over the vector instead (prefer new overload)?

In other words, should anything convertible to std::unordered_set still behave as std::unordered_set, or should it pick the possibly-new overload according to regular language rules?

I think we should pick the former (full backwards compatibility), but I'd like some confirmation before I dive into SFINAE complexity etc.

@ihsandemir
Copy link
Collaborator

Please look at the impl. at here. As you see, as long as we can iterate the container and the entry has the .first and .second available, then it will compile and work as expected. Hence, you do not need to and should not convert to unordered_map etc.

I would imagine the API would be something like this:

    template<typename CONTAINER>
    boost::future<void> put_all(const CONTAINER& entries)

@ihsandemir
Copy link
Collaborator

For not breaking existing user code, all our existing tests which use imap::put_all should work without any modification to the tests. you can also add more tests.

@ldrozdz93
Copy link
Author

Thanks, so I'll use the current test cases as a reference, without coming up with any new cases of what backwards compatibility should mean.

@ihsandemir
Copy link
Collaborator

Please do not forget to put the documentation update and the code examples update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants