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

C++ code cleanup and modernization #1452

Merged
merged 13 commits into from
Mar 9, 2023
Merged

Conversation

speth
Copy link
Member

@speth speth commented Mar 7, 2023

This is some wide-ranging cleanup of the code to make use of some features from the modern C++ to make the code simpler and more readable.

Given the large number of files this touches, I know there will be a few merge conflicts, so I'm happy to try getting a couple of existing PRs merged ahead of this one and deal with those conflicts here.

Changes proposed in this pull request

  • Use if constexpr (C++17) to greatly simplify the implementation of MultiRate methods that depend on whether or not the rate class implements specific methods like updateFromStruct
  • Use std::filesystem (C++17) to avoid OS-specific implementations of get_modified_time
  • Use "structured bindings" C++17 to unpack std::pair/std::tuple and improve readability (for example, for (auto& [a, b] : some_map))
  • Make use of C++11 "non-static data member initialization" to initialize member variables in the class definition rather than in the constructor, whenever possible.
  • Make use of make_unique (C++14) and make_shared (C++11) to reduce usage of bare new.
  • Use ClassName() = default; to implement trivial constructors and similar for destructors (C++11)
  • Remove unnecessary standard library includes and using statements
  • Use std::any (C++17) instead of boost::any to implement AnyValue. Since we no longer need to hide the boost type behind a unique_ptr, this simplifies the implementation of AnyValue slightly and eliminates an extra memory allocation for every AnyValue object.

Checklist

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #1452 (eafc669) into main (399e1cb) will decrease coverage by 1.40%.
The diff coverage is 82.09%.

@@            Coverage Diff             @@
##             main    #1452      +/-   ##
==========================================
- Coverage   71.02%   69.63%   -1.40%     
==========================================
  Files         369      373       +4     
  Lines       55558    55769     +211     
  Branches    18378    18295      -83     
==========================================
- Hits        39462    38833     -629     
- Misses      13607    14480     +873     
+ Partials     2489     2456      -33     
Impacted Files Coverage Δ
include/cantera/base/ExtensionManager.h 27.27% <ø> (ø)
include/cantera/base/FactoryBase.h 72.72% <ø> (+0.85%) ⬆️
include/cantera/base/NoExitLogger.h 0.00% <ø> (ø)
include/cantera/base/Units.h 70.00% <ø> (ø)
include/cantera/base/utilities.h 96.42% <ø> (ø)
include/cantera/equil/MultiPhaseEquil.h 90.47% <ø> (ø)
include/cantera/kinetics/GasKinetics.h 100.00% <ø> (ø)
include/cantera/kinetics/ReactionRate.h 76.36% <ø> (-2.89%) ⬇️
include/cantera/numerics/BandMatrix.h 0.00% <ø> (ø)
include/cantera/numerics/CVodesIntegrator.h 31.25% <ø> (-4.47%) ⬇️
... and 377 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

Thanks for this extensive cleanup, @speth! The changes are straight-forward and all look good to me. I definitely like the unpacking of pairs, which definitely makes code easier to follow!

I will approve once the remaining Intel compiler failure is resolved.

Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

Btw, feel free to cherry-pick 0973ae2 from #1451 ... it's a better fit here.

@ischoegl
Copy link
Member

ischoegl commented Mar 8, 2023

@speth ... on a related note: what are differences between boost's any and the new std::any introduced in C++17? Is this a drop-in replacement? Edit: here's a link to a related SO question

@speth
Copy link
Member Author

speth commented Mar 8, 2023

Yes, std::any is basically the adoption of boost::any into the standard library. I had started testing a transition, but it doesn't really have much of a benefit at the moment -- no impact on compilation time, and we still depend on Boost either way. Still, it might save some headache down the line.

@ischoegl
Copy link
Member

ischoegl commented Mar 8, 2023

Yes, std::any is basically the adoption of boost::any into the standard library. I had started testing a transition, but it doesn't really have much of a benefit at the moment -- no impact on compilation time, and we still depend on Boost either way. Still, it might save some headache down the line.

If I'm not mistaken, the std::any implementation may have some optimizations for small objects, so it still may pay off. Not sure whether this has any measurable impact on performance though ...

@speth speth force-pushed the cxx17-features branch 3 times, most recently from 3410095 to d63895c Compare March 9, 2023 04:57
speth added 2 commits March 9, 2023 10:29
Since we no longer need to "hide" the Boost usage, we can make the
std::any member of AnyValue a regular member variable rather than
a pointer, which simplifies a few things.
Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

Thank you for the updates!

@ischoegl ischoegl merged commit 5d247d0 into Cantera:main Mar 9, 2023
@speth speth deleted the cxx17-features branch March 20, 2023 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

2 participants