LLNL / ygm

Other
31 stars 22 forks source link

Added support for (key, value) lambdas while still supporting (kv_pair) lambdas for ygm::container::map #123

Closed bwpriest closed 1 year ago

bwpriest commented 1 year ago

I've added caveats throughout, but it bears repeating here. The changes are backwards-compatible except in one specific way: It is no longer possible to use auto for a visitor with an optional pointer argument and no other arguments. That is,

[](auto pmap, auto kv_pair) {}

will no longer compile correctly, as the constexpr std::is_invocable check is unable to distinguish this from a key-value lambda with no optional pointer argument. Consequently, to use a lambda of this type one must specify that the second argument is a pair, i.e.

[](auto pmap, std::pair<auto, auto> kv_pair) {}

or something similar. I thought providing backwards compatibility with this caveat was better than forcing all dependent codes to update their lambdas.

bwpriest commented 1 year ago

It is no longer possible to use auto for a visitor with an optional pointer argument and no other arguments.

Actually, this is true even if there are other arguments. The compiler cannot distinguish between lambdas with the signature [](auto x, auto y, ...){} where x and y are intended to be a pointer and a key-value pair and lambdas with the same signature where x is intended to be a key and y is intended to be a value. If using an optional pointer, it is necessary to declare the second argument as std::pair<auto, auto> or similar.

bwpriest commented 1 year ago

@steiltre

bwpriest commented 1 year ago

I have not done robust testing to verify that this does not cause any problems with ygm::container::multimap or ygm::container::counting_set. All of the tests pass, but there could be sharp edges for which I've not accounted.

steiltre commented 1 year ago

These changes look good to me. Thanks for being explicit about when auto deduces the wrong types. I like the assumption that we go with my_lambda(auto x, auto y) meaning my_lambda(const Key &x, Value &y), as you have done.

We should slowly try to convert our codebases to use the new lambdas with split keys and values. Eventually, it may be worth deprecating the legacy lambdas for consistency.

bwpriest commented 1 year ago

Eventually, it may be worth deprecating the legacy lambdas for consistency.

I concur, but I didn't want to force the issue until we're ready to release a new version.