insert_check and callees should pass KeyValueCompare by const reference

[vinnie.falco@…]

boost::intrusive::set::insert_check passes KeyValueCompare by value. And then it makes another copy when it passes it by value to the underlying tree_type. As KeyValueCompare could be expensive to copy, it seems needless to pass it by value. It should be passed by const reference. This applies to all containers with insert_check.

[vinnie.falco@…]

It looks like this is applicable to any member function that has a template parameter KeyValueCompare, like find.

[Ion Gaztañaga]

Thanks for the report. You can always wrap your function objects with a type that holds a reference to your heavyweight function object. Passing by const-reference forces operator() to be const which is not required right now (and thus, this is backwards incompatible). For other function objects used in Intrusive, like cloners and disposers, non-const operator() is very useful to recycle nodes.

Usually function in the C++ standard algorithms predicates are passed as values for very good reasons:

​http://stackoverflow.com/questions/8196345/passing-functor-object-by-value-vs-by-reference-c

I think you can always obtain the desired performance with a little wrapper. Passing by value is the intended behavior in Intrusive. Maybe not optimal in some cases, but this is by design.

[vinnie.falco@…]

That's a fair point, and one that I hadn't considered. Changing my calls to insert_commit to use std::ref() for the functor solved the problem.

The reason that I brought it up is because there is a certain asymmetry in the API. When you construct a container with a custom Compare object and initialize it in the constructor, it because a data member of the container and is never copied when used in functions like the find() overloads which do not take an additional KeyValueCompare parameter. Compare this with the signatures of the overloads which do take the KeyValueCompare by value.

However, this asymmetry is the lesser of the evils, especially since the reference wrapper is a very acceptible workaround. Thanks