Get rid of nested std::bind, and some boost removal#152
Get rid of nested std::bind, and some boost removal#152sean-parent merged 2 commits intostlab:mainfrom
Conversation
venik
changed the title
| return adobe::sort( | ||
| boost::begin(r), boost::end(r), | ||
| std::bind(c, std::bind(p, std::placeholders::_1), std::bind(p, std::placeholders::_2))); | ||
| return adobe::sort(boost::begin(r), boost::end(r), c, p); |
There was a problem hiding this comment.
Can we avoid boost here?
| return adobe::sort(boost::begin(r), boost::end(r), c, p); | |
| return adobe::sort(std::begin(r), std::end(r), c, p); |
There was a problem hiding this comment.
we could, but that will come at a price
std::vector<int> vec = ...
std::pair<std::vector<int>::iterator, std::vector<int>::iterator> pair = std::make_pair(vec.begin(), vec.end());
adobe::sort(pair, ...)
this will not work, std::begin()/end doesn't work with pair, and passing pair of iterators is extremely effective. None of it will matter with c++20 and ranges, but before that it's useful
There was a problem hiding this comment.
Thank you for that clarification. The changes LGTM - lambdas are much easier to read than bind! I'll defer to @sean-parent to give the final approval.
sean-parent
left a comment
sean-parent
left a comment
There was a problem hiding this comment.
Some issues noted in comments. Generally, I approve of the transformation, but care must be taken to not escape items captured by reference unless the prior implementation was capturing by reference.
source/adam.cpp
Outdated
| cell_set_m.push_back(cell_t( | ||
| access_output, name, | ||
| std::bind(&implementation_t::calculate_expression, std::ref(*this), position, expression), | ||
| [&, this]() { return calculate_expression(position, expression); }, |
There was a problem hiding this comment.
The expression is escaping. In the original bind position and expression are captured by value - capturing them by reference is dangerous and may be escaping a temporary. I believe this should be [=, this]. I also prefer being explicit about the captures so [_position = position, _expression = expression, this] would be preferred.
adobe/algorithm/gather.hpp
Outdated
| std::stable_partition(pivot, last, std::bind(pred, std::placeholders::_1))); | ||
| [&](auto&& v) -> bool { return !pred(std::forward<decltype(v)>(v)); }), | ||
| std::stable_partition(pivot, last, [&](auto&& v) -> bool { | ||
| return pred(std::forward<decltype(v)>(v)); |
There was a problem hiding this comment.
pred() needs to be called with std::invoke to match the prior implementation and support. v should be passed as const&
source/eve_evaluate.cpp
Outdated
| std::bind(proc, _1, _3, std::bind(&evaluate_named_arguments, std::ref(evaluator), _4)); | ||
| suite.add_cell_proc_m = std::bind(&add_cell, std::ref(sheet), _1, _2, _3, _4); | ||
| suite.add_relation_proc_m = std::bind(&add_relation, std::ref(sheet), _1, _2, _3, _4); | ||
| [&evaluator, &proc](const eve_callback_suite_t::position_t& parent, const line_position_t& /* parse_location */, |
There was a problem hiding this comment.
The prior call captured proc by value; you have changed it to by reference.
|
LGTM |
3 participants
std::bind in MSVC can generate deeply nested template instantiations when you chain or nest binds, especially with lambdas or other std::bind calls inside. The complexity can grow exponentially with the number of bound arguments and placeholders, leading to very slow compilation and large binaries. Also replaced some boost type_traits with std counterparts.
can be compiled with c++17