Re: Review Request 34645: Update existing lambdas to meet style guide
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34645/#review87149 --- Ship it! Ship It! - Benjamin Hindman On June 1, 2015, 5:26 p.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34645/ --- (Updated June 1, 2015, 5:26 p.m.) Review request for mesos, Benjamin Hindman and Joris Van Remoortere. Bugs: MESOS-2670 https://issues.apache.org/jira/browse/MESOS-2670 Repository: mesos Description --- Update existing lambdas to meet style guide Diffs - src/master/master.cpp 710b8149c9d855d0f47cb2952366be10bc78c74d Diff: https://reviews.apache.org/r/34645/diff/ Testing --- Thanks, haosdent huang
Re: Review Request 34645: Update existing lambdas to meet style guide
On May 25, 2015, 11:05 p.m., Joris Van Remoortere wrote: This is an interesting case. We have a proxy to another function, rather than the implementation of that function as a lambda. I'm curious what the community's view is on using the proxy lambda approach as per your patch, versus a `std::bind`. I think the lambda is more readable, the bind is more explicit about what is going on :-) Depending on the way the community votes, I would add a comment here just stating that this is a proxy lambda. What do you think? haosdent huang wrote: I think it would be better to discard this patch. And I think it would better to convert some lambda::bind which don't have complex params. For most lambda::bind in current code, their parameters could not copy safety. So use lambda::bind and keep current code maybe better. But for some lambda::bind, if it's paramters are basic types and change it could import readbility, I think it would be better to convert them. Joris Van Remoortere wrote: I discussed this earlier today with BenH and some others. We think your strategy is a good one. Let's keep this patch, and just update it with the outstanding issues. Just add a comment stating this is a proxy lambda? haosdent huang wrote: Sorry for no reply for a long time because of busy at my current work. I still prefer to discard this one if change it here could bring some problems. And I would change some other lambdas::bind which don't have so much affects (eg. don't contains params). Hi, @jvanremoortere . I update the patch, could you review it again? Thank you very much. - haosdent --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34645/#review85137 --- On June 1, 2015, 5:26 p.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34645/ --- (Updated June 1, 2015, 5:26 p.m.) Review request for mesos, Benjamin Hindman and Joris Van Remoortere. Bugs: MESOS-2670 https://issues.apache.org/jira/browse/MESOS-2670 Repository: mesos Description --- Update existing lambdas to meet style guide Diffs - src/master/master.cpp 710b8149c9d855d0f47cb2952366be10bc78c74d Diff: https://reviews.apache.org/r/34645/diff/ Testing --- Thanks, haosdent huang
Re: Review Request 34645: Update existing lambdas to meet style guide
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34645/ --- (Updated June 1, 2015, 5:24 p.m.) Review request for mesos, Benjamin Hindman and Joris Van Remoortere. Bugs: MESOS-2670 https://issues.apache.org/jira/browse/MESOS-2670 Repository: mesos Description --- Update existing lambdas to meet style guide Diffs (updated) - src/master/master.cpp 710b8149c9d855d0f47cb2952366be10bc78c74d Diff: https://reviews.apache.org/r/34645/diff/ Testing --- Thanks, haosdent huang
Re: Review Request 34645: Update existing lambdas to meet style guide
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34645/ --- (Updated June 1, 2015, 5:26 p.m.) Review request for mesos, Benjamin Hindman and Joris Van Remoortere. Bugs: MESOS-2670 https://issues.apache.org/jira/browse/MESOS-2670 Repository: mesos Description --- Update existing lambdas to meet style guide Diffs (updated) - src/master/master.cpp 710b8149c9d855d0f47cb2952366be10bc78c74d Diff: https://reviews.apache.org/r/34645/diff/ Testing --- Thanks, haosdent huang
Re: Review Request 34645: Update existing lambdas to meet style guide
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34645/#review86056 --- Patch looks great! Reviews applied: [34644, 34645] All tests passed. - Mesos ReviewBot On June 1, 2015, 5:26 p.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34645/ --- (Updated June 1, 2015, 5:26 p.m.) Review request for mesos, Benjamin Hindman and Joris Van Remoortere. Bugs: MESOS-2670 https://issues.apache.org/jira/browse/MESOS-2670 Repository: mesos Description --- Update existing lambdas to meet style guide Diffs - src/master/master.cpp 710b8149c9d855d0f47cb2952366be10bc78c74d Diff: https://reviews.apache.org/r/34645/diff/ Testing --- Thanks, haosdent huang
Re: Review Request 34645: Update existing lambdas to meet style guide
On May 25, 2015, 11:05 p.m., Joris Van Remoortere wrote: This is an interesting case. We have a proxy to another function, rather than the implementation of that function as a lambda. I'm curious what the community's view is on using the proxy lambda approach as per your patch, versus a `std::bind`. I think the lambda is more readable, the bind is more explicit about what is going on :-) Depending on the way the community votes, I would add a comment here just stating that this is a proxy lambda. What do you think? haosdent huang wrote: I think it would be better to discard this patch. And I think it would better to convert some lambda::bind which don't have complex params. For most lambda::bind in current code, their parameters could not copy safety. So use lambda::bind and keep current code maybe better. But for some lambda::bind, if it's paramters are basic types and change it could import readbility, I think it would be better to convert them. Joris Van Remoortere wrote: I discussed this earlier today with BenH and some others. We think your strategy is a good one. Let's keep this patch, and just update it with the outstanding issues. Just add a comment stating this is a proxy lambda? Sorry for no reply for a long time because of busy at my current work. I still prefer to discard this one if change it here could bring some problems. And I would change some other lambdas::bind which don't have so much affects (eg. don't contains params). - haosdent --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34645/#review85137 --- On May 24, 2015, 4:53 p.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34645/ --- (Updated May 24, 2015, 4:53 p.m.) Review request for mesos, Benjamin Hindman and Joris Van Remoortere. Bugs: MESOS-2670 https://issues.apache.org/jira/browse/MESOS-2670 Repository: mesos Description --- Update existing lambdas to meet style guide Diffs - src/master/master.cpp 1526f59e7c6b135657550eab2ca46216923a01f6 Diff: https://reviews.apache.org/r/34645/diff/ Testing --- Thanks, haosdent huang
Re: Review Request 34645: Update existing lambdas to meet style guide
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34645/#review85137 --- This is an interesting case. We have a proxy to another function, rather than the implementation of that function as a lambda. I'm curious what the community's view is on using the proxy lambda approach as per your patch, versus a `std::bind`. I think the lambda is more readable, the bind is more explicit about what is going on :-) Depending on the way the community votes, I would add a comment here just stating that this is a proxy lambda. What do you think? src/master/master.cpp https://reviews.apache.org/r/34645/#comment136612 1) I don't think this will compile. We need to capture `this` rather than `allocator` if we want to access a class member of the member function. 2) We can use `string` here rather than `std::string` since we are in an implementation file that has declared `using std::string` - Joris Van Remoortere On May 24, 2015, 4:53 p.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34645/ --- (Updated May 24, 2015, 4:53 p.m.) Review request for mesos, Benjamin Hindman and Joris Van Remoortere. Bugs: MESOS-2670 https://issues.apache.org/jira/browse/MESOS-2670 Repository: mesos Description --- Update existing lambdas to meet style guide Diffs - src/master/master.cpp 1526f59e7c6b135657550eab2ca46216923a01f6 Diff: https://reviews.apache.org/r/34645/diff/ Testing --- Thanks, haosdent huang
Review Request 34645: Update existing lambdas to meet style guide
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34645/ --- Review request for mesos, Benjamin Hindman and Joris Van Remoortere. Bugs: MESOS-2670 https://issues.apache.org/jira/browse/MESOS-2670 Repository: mesos Description --- Update existing lambdas to meet style guide Diffs - src/master/master.cpp 1526f59e7c6b135657550eab2ca46216923a01f6 Diff: https://reviews.apache.org/r/34645/diff/ Testing --- Thanks, haosdent huang
