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

Jump to bottom

clang-tidy update #3895

Merged
merged 5 commits into from Nov 20, 2020
Merged

clang-tidy update #3895

merged 5 commits into from Nov 20, 2020

Conversation

tjhei
Copy link
Member

@tjhei tjhei commented Nov 18, 2020

  • update clang-tidy rules (taken from deal.II)
  • run with new clang-tidy 10
  • update and fix contrib/utilities/run_clang_tidy.sh
  • apply fixes:
  1. replace bind() by lambdas
  2. use range for loop
  3. various smaller fixes

@tjhei
update clang-tidy settings and run script
328c8ec 
- update to new clang-tidy-10 settings
- disable unity/pch support in script
tjhei
tjhei commented Nov 18, 2020
View reviewed changes
source/simulator/core.cc Outdated
prescribed_stokes_solution (PrescribedStokesSolution::create_prescribed_stokes_solution<dim>(prm)),
adiabatic_conditions (AdiabaticConditions::create_adiabatic_conditions<dim>(prm)),
[&]()
{
Copy link
Member Author

Choose a reason for hiding this comment

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

astyle sadly gets confused with the indentation here. Should we leave the bind() call in here?

Copy link
Member

Choose a reason for hiding this comment

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

Is there a real downside of using bind? If not I would personally prefer the readability for now. Also in the other places where bind has been replaced.

@tjhei tjhei changed the title Clang tidy update clang-tidy update Nov 19, 2020
MFraters
MFraters reviewed Nov 20, 2020
View reviewed changes
Copy link
Member

@MFraters MFraters left a comment

Choose a reason for hiding this comment

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

Wow, those are a lot of changes. I really like to loop modernizations. Did clang-tidy autofix the code or did you have to fix it all by hand?

I do have a few small comments/questions, but I am generally fine with it being merged.

source/main.cc Outdated Show resolved Hide resolved
source/material_model/rheology/drucker_prager.cc Outdated Show resolved Hide resolved
source/material_model/steinberger.cc Outdated
@@ -195,8 +195,8 @@ namespace aspect
if (use_lateral_average_temperature)
{
this->get_lateral_averaging().get_temperature_averages(avg_temp);
for (unsigned int i = 0; i < avg_temp.size(); ++i)
AssertThrow(numbers::is_finite(avg_temp[i]),
for (double i : avg_temp)
Copy link
Member

Choose a reason for hiding this comment

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

hmm, I think the avg_temp should be average_temperatures and i should be average_temperature. I will stop marking these out for the rest of the review because I think the point is clear and since this is strictly speaking only addressing the tidy comments it might be out of scope of this pull request.

Copy link
Member Author

Choose a reason for hiding this comment

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

avg_temp is a class member. Maybe we do this separately after this PR is merged?

source/postprocess/heat_flux_statistics.cc Outdated Show resolved Hide resolved
source/postprocess/visualization.cc Outdated Show resolved Hide resolved
source/simulator/core.cc Outdated
prescribed_stokes_solution (PrescribedStokesSolution::create_prescribed_stokes_solution<dim>(prm)),
adiabatic_conditions (AdiabaticConditions::create_adiabatic_conditions<dim>(prm)),
[&]()
{
Copy link
Member

Choose a reason for hiding this comment

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

Is there a real downside of using bind? If not I would personally prefer the readability for now. Also in the other places where bind has been replaced.

@tjhei
apply fixes
752fb6d 
undo boost bind change
@tjhei
Copy link
Member Author

tjhei commented Nov 20, 2020

Is there a real downside of using bind?

Not really. I agree, let's keep it for now. I updated the patch with (most of) your suggestions.

@tjhei
Copy link
Member Author

tjhei commented Nov 20, 2020

Did clang-tidy autofix the code or did you have to fix it all by hand?

This uses the -fix setting of clang-tidy with some manual cleanup (variable names).

MFraters
MFraters approved these changes Nov 20, 2020
View reviewed changes
Copy link
Member

@MFraters MFraters left a comment

Choose a reason for hiding this comment

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

looks good to me.

avg_temp is a class member. Maybe we do this separately after this PR is merged?

Sure, and since I am the one nitpicking here, I will put in the effort to make the change ;)

@MFraters MFraters merged commit e383a30 into geodynamics:master Nov 20, 2020
@tjhei tjhei deleted the clang-tidy-update branch November 20, 2020 21:43
@MFraters MFraters mentioned this pull request Nov 22, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MFraters MFraters MFraters approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tjhei @MFraters