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

clang-tidy option in cmake an fix for cpp 14 #293

Merged
merged 6 commits into from
Jun 28, 2021

Conversation

MFraters
Copy link
Member

This adds an option to run the clang-tidy check when compiling. It is off by default, because it makes compiling slower and is only useful when developing (could turn it on in debug mode in the future). I also am rerunning and fixing a select set of clang-tidy checks clang-tidy now that I am using cpp14. I am not sure whether all changes have to do with c++14 or I just missed them before.

I am also considering adding a test whether there are any issues.

@codecov
Copy link

codecov bot commented Jun 27, 2021

Codecov Report

Merging #293 (d515960) into master (be243ea) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #293   +/-   ##
=======================================
  Coverage   99.00%   99.00%           
=======================================
  Files          74       74           
  Lines        4228     4228           
=======================================
  Hits         4186     4186           
  Misses         42       42           
Impacted Files Coverage Δ
source/coordinate_systems/cartesian.cc 100.00% <ø> (ø)
source/coordinate_systems/interface.cc 100.00% <ø> (ø)
source/coordinate_systems/spherical.cc 100.00% <ø> (ø)
.../continental_plate_models/composition/interface.cc 100.00% <ø> (ø)
...es/continental_plate_models/composition/uniform.cc 100.00% <ø> (ø)
...tures/continental_plate_models/grains/interface.cc 100.00% <ø> (ø)
...eatures/continental_plate_models/grains/uniform.cc 100.00% <ø> (ø)
.../continental_plate_models/temperature/adiabatic.cc 94.11% <ø> (ø)
.../continental_plate_models/temperature/interface.cc 100.00% <ø> (ø)
...res/continental_plate_models/temperature/linear.cc 100.00% <ø> (ø)
... and 64 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be243ea...d515960. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Jun 27, 2021

Benchmark Master Feature Difference (99.9% CI)
Slab interpolation simple none 1.472 ± 0.089 (s=304) 1.457 ± 0.077 (s=313) -2.5% .. +0.5%
Slab interpolation curved simple none 1.400 ± 0.006 (s=314) 1.399 ± 0.003 (s=332) -0.1% .. +0.0%
Spherical slab interpolation simple none 2.032 ± 0.042 (s=213) 2.034 ± 0.045 (s=232) -0.5% .. +0.8%
Slab interpolation simple curved CMS 2.131 ± 0.017 (s=222) 2.126 ± 0.015 (s=203) -0.5% .. +0.0%
Spherical slab interpolation simple CMS 4.463 ± 0.163 (s=108) 4.457 ± 0.182 (s=96) -2.0% .. +1.7%
Spherical fault interpolation simple none 2.704 ± 0.047 (s=174) 2.692 ± 0.042 (s=162) -1.0% .. +0.2%

@MFraters MFraters force-pushed the clang_tidy_cpp14 branch 3 times, most recently from c7bdab7 to f167391 Compare June 27, 2021 06:07
@MFraters
Copy link
Member Author

I think these changes are sensible and considerably clean up the code. I also think they best to do right after the release, so that future development for new contributors will hopefully be easier. Although I do not think will make a real git merge conflict, I will wait on merging for #289 since it is nearly almost ready to merge and it includes some header files which will be changed. I don't think I need to wait for #264, since the changes there are not close the the changes in this pull request.

In summary, this modernizes the code for cpp14 in several areas and introduces a few readability improvements. They can be automatically checked and even fixed though cmake, but I don't think I will enforce this strictly for the time being. There is a will also be a follow up pull request to clean the code up even more (remove unused headers and fix some clang warnings (I will also need to add a non-mac clang tester, they are really different in their warnings apparently))

@MFraters MFraters added the ready to merge Pull request is ready to merge. May be waiting for tests to complete or other reviews. label Jun 27, 2021
@MFraters MFraters merged commit bdb5458 into GeodynamicWorldBuilder:master Jun 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull request is ready to merge. May be waiting for tests to complete or other reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant