-
Notifications
You must be signed in to change notification settings - Fork 299
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
Initial review and refactoring of C++ code #2104
base: master
Are you sure you want to change the base?
Conversation
a3b3f5e
to
5b61bba
Compare
(In case that isn't clear, the new github actions did their job and told me I had broken mac and windows (I'm on Ubuntu right now), which I am currently fixing) |
before: https://github.com/jmarrec/CoolProp/runs/5722400572?check_suite_focus=true
after: https://github.com/jmarrec/CoolProp/runs/5724011199?check_suite_focus=true
|
b2bb2b1
to
4850fa5
Compare
… + Strip trailing whitespaces ``` find . -regextype posix-extended -regex '.*\.(cpp|hpp|c|h|cxx|hxx|py)$' | xargs -I@ sed -i 's/[ \t]*$//' "@" ```
This looks good to me. Given that the PR is quite old, is there anything that needs updating? I am currently trying to stitch up a new release... |
@jowr I am not planning on doing more changes at least, if that's what you're asking. PS: In fact I'm not either planning to do any changes to the state of this PR either. I don't want it to be understood as being harsh or whatnot, I just volunteered an enormous amount of time to this PR, the python packaging and github actions stuff, for a library I actually do not use, and time is not a luxury I have at all anymore. |
No worries - we are all volunteers here and I am just trying to create a new release to put all your valuable changes to work. As you can see, I also do not have the time that I would love to spend on this project (sorry for the late reply). I'll see what I can do integrating it with the latest master. |
I am actively working on merging this soon. @jmarrec, could you please sign the CLA as mentioned here: #2104 (comment) ? Thank you once again for all the efforts. |
@jowr done |
@@ -15,7 +15,7 @@ namespace CoolProp { | |||
|
|||
struct SimpleState | |||
{ | |||
double rhomolar, T, p, hmolar, smolar, umolar, Q; | |||
double rhomolar{}, T{}, p{}, hmolar{}, smolar{}, umolar{}, Q{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be some problem with the examples. I have not looked at this in detail, yet.
https://github.com/CoolProp/CoolProp/actions/runs/3735565861/jobs/6592680936#step:8:145
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this just forces default initialization, no?
Requirements
I didn't go everywhere. And I left some
TODO
s (search for TODO) for maintainers.Note: this builds upon the
lint
branch, so merge #2103 and the diffs will get cleaned up, as can be seen here: jmarrec/CoolProp@lint...cpp_reviewDescription of the Change
Refactor the code where appropriate and review it.
Benefits
Try to use modern C++. For eg the entire project is using deprecated headers (eg:
stdlib.h
instead of<cstdlib>
). Lots of range-based for loops can be applied, some move semantics. Virtual empty destructors were completely unecessary, overuse of C arrays, etc.Possible Drawbacks
Verification Process
Applicable Issues
[ Enter any applicable Issues here. Use
Closes #????
if this PR closes an open issue. ]