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

Standardize code cont'd #1568

Merged
merged 15 commits into from Aug 5, 2023
Merged

Standardize code cont'd #1568

merged 15 commits into from Aug 5, 2023

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Aug 3, 2023

Changes proposed in this pull request

Implement remaining items from Cantera/enhancements#181

If applicable, fill in the issue number this pull request is fixing

Closes Cantera/enhancements#181

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

@ischoegl ischoegl force-pushed the standardize-code-contd branch 2 times, most recently from fa74984 to ecd010c Compare August 3, 2023 23:23
@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Merging #1568 (0622402) into main (e0a2f11) will increase coverage by 0.03%.
The diff coverage is 82.70%.

@@            Coverage Diff             @@
##             main    #1568      +/-   ##
==========================================
+ Coverage   70.50%   70.53%   +0.03%     
==========================================
  Files         379      379              
  Lines       59112    59110       -2     
  Branches    21232    21232              
==========================================
+ Hits        41675    41695      +20     
+ Misses      14361    14340      -21     
+ Partials     3076     3075       -1     
Files Changed Coverage Δ
include/cantera/base/ExtensionManager.h 27.27% <0.00%> (ø)
include/cantera/base/Interface.h 100.00% <ø> (ø)
include/cantera/base/NoExitLogger.h 0.00% <0.00%> (ø)
include/cantera/base/ValueCache.h 100.00% <ø> (ø)
include/cantera/base/YamlWriter.h 100.00% <ø> (ø)
include/cantera/equil/vcs_MultiPhaseEquil.h 100.00% <ø> (ø)
include/cantera/equil/vcs_SpeciesProperties.h 100.00% <ø> (ø)
include/cantera/kinetics/MultiRateBase.h 100.00% <ø> (ø)
include/cantera/kinetics/ReactionData.h 67.85% <0.00%> (ø)
include/cantera/kinetics/solveSP.h 100.00% <ø> (ø)
... and 258 more

... and 2 files with indirect coverage changes

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

@ischoegl ischoegl marked this pull request as ready for review August 4, 2023 00:15
@ischoegl ischoegl requested a review from a team August 4, 2023 00:15
@ischoegl
Copy link
Member Author

ischoegl commented Aug 4, 2023

@speth ... per request, I added all remaining 'standardizations' in a single PR. I did not touch include/cantera/numerics/ctlapack.h, which continues to use doublereal.

@ischoegl ischoegl removed the request for review from a team August 4, 2023 00:45
@ischoegl ischoegl marked this pull request as draft August 4, 2023 00:46
@ischoegl ischoegl marked this pull request as ready for review August 4, 2023 01:42
@ischoegl ischoegl requested a review from a team August 4, 2023 01:43
@ischoegl
Copy link
Member Author

ischoegl commented Aug 4, 2023

Rebased to resolve merge conflict after #1570

This code isn't part of namespace Cantera, so can't use the
typedefs that are specified in that namespace.
Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

I added a couple of commits to:

  1. fix compilation of the Matlab toolbox, which defines some functions outside of the Cantera namespace and therefore doesn't inherit the using statements from that namespace.
  2. Do some opportunistic formatting cleanup where the shorter names allowed it. I figured just pushing this change was more efficient than leaving 100 comments on this PR.

Is there a reason for not specifically deprecating doublereal and updating ctlapack.h? I suspect the rationale described in config.h.in is a little bit misguided -- I don't think there's any way for for the LAPACK/Fortran double precision type to be anything other than double, and even if this was possible, I don't think it could be handled by a simple typedef.

@ischoegl
Copy link
Member Author

ischoegl commented Aug 5, 2023

Thanks for pushing corrections/updates directly, @speth ... didn't think about Matlab not using the namespace / ct_defs.h. Regarding reformatting, I did a regex search for the most obvious changes, but apparently missed numerous spots. Probably I was too opportunistic ... .

Regarding leaving doublereal in ctlapack.h, I wasn't sure about the history, and decided to leave it (in part also because of the comment in config.h.in you mentioned). I'm not too concerned about doublereal surviving a little longer, as I'm sure it makes no difference to the compiler.

Unless you have other suggestions, this is good to go from my side. Let's see what merge conflicts will arise in #1567 (should be easy to clean up though).

@speth speth merged commit 3332319 into Cantera:main Aug 5, 2023
42 checks passed
@ischoegl ischoegl deleted the standardize-code-contd branch August 5, 2023 20:29
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.

Remove unnecessary typedef's
2 participants