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

Remove std::bind from examples #8891

Merged
merged 7 commits into from
Oct 19, 2019
Merged

Conversation

masterleinad
Copy link
Member

This pull request removes all the uses of std::bind from the examples. I am happy for suggestions for improving the documentation. Currently, I am removing all occurences of std::bind also in the documentation for the examples but would also be fine to mention it as long as we encourage using lambdas.

@masterleinad
Copy link
Member Author

/rebuild

Copy link
Member

@bangerth bangerth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question (not even a suggestion) of how we want to deal with captures in the tutorial programs.

AssemblyCopyData & copy_data) {
this->local_assemble_matrix(cell, scratch_data, copy_data);
},
[this, &linear_system](const AssemblyCopyData &copy_data) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is in a tutorial program where simplicity is a concern, what would you think about omitting the explicit captures here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel that listing the captures is a big issue, but I find it a lot more readable if you move the definition of the lambda above the function call and give it a descriptive name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reasoning for the explicit capture list would be that we make it as clear as possible what the lambda is really doing (and which variables it is using).
I don't have a strong opinion here, though. If someone strongly prefers one variant I am happy to change all of them in the examples.

I think that we should discuss both variants here in any way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also see http://open-std.org/JTC1/SC22/WG21/docs/papers/2018/p0806r2.html and https://en.cppreference.com/w/cpp/language/lambda:

 The implicit capture of *this when the capture default is = is deprecated. (since C++20).

examples/step-13/step-13.cc Outdated Show resolved Hide resolved
AssemblyCopyData & copy_data) {
this->local_assemble_matrix(cell, scratch_data, copy_data);
},
[this, &linear_system](const AssemblyCopyData &copy_data) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

std::ref(face_integrals)),
[this,
&error_indicators,
&face_integrals](const active_cell_iterator & cell,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here

Assembly::CopyData::StokesPreconditioner<dim> & data) {
this->local_assemble_stokes_preconditioner(cell, scratch, data);
},
[this](const Assembly::CopyData::StokesPreconditioner<dim> &data) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here

&BoussinesqFlowProblem<dim>::copy_local_to_global_temperature_rhs,
this,
std::placeholders::_1),
[this, global_T_range, maximal_velocity, global_entropy_variation](
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and in particular here

Copy link
Member

@bangerth bangerth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make the minor changes and merge yourself.

I think my question about how explicit we want to be with the capture comes from the feeling that most of our users will only ever have seen lambdas that start with [] and simply capture everything. I suspect that a capture list isn't in their vocabulary, and it's not like the syntax of lambda functions isn't already complicated enough...

unsigned int & data) {
this->postprocess_one_cell(cell, scratch, data);
},
[](const unsigned int &) {},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, this is a severe degradation here. WorkStream::run checks whether the copied is an empty function object std::function<void(const unsigned int &)>() and uses a far more efficient algorithm in that case. An empty lambda function is not the same as the empty function argument.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue here as before: you really do want the do_nothing_copier here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... yes, we need to document this (internal) peculiarity.

examples/step-13/step-13.cc Outdated Show resolved Hide resolved
examples/step-13/step-13.cc Outdated Show resolved Hide resolved
examples/step-14/step-14.cc Outdated Show resolved Hide resolved
examples/step-14/step-14.cc Outdated Show resolved Hide resolved
examples/step-32/step-32.cc Outdated Show resolved Hide resolved
masterleinad and others added 2 commits October 18, 2019 10:48
Co-Authored-By: Wolfgang Bangerth <bangerth@colostate.edu>
Co-Authored-By: Wolfgang Bangerth <bangerth@colostate.edu>
@masterleinad
Copy link
Member Author

@bangerth Would you mind approving this "officially" since you already gave your consent in the comments? Pull request authors can't approve their own pull requests.

@bangerth bangerth merged commit fd20c38 into dealii:master Oct 19, 2019
@masterleinad masterleinad deleted the remove_bind_4 branch October 19, 2019 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants