Initial review and refactoring of C++ code #2104
Conversation
|
|
jmarrec
force-pushed
the
cpp_review
branch
3 times, most recently
from
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 |
jmarrec
force-pushed
the
cpp_review
branch
3 times, most recently
from
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.
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.
I think this just forces default initialization, no?
Requirements
I didn't go everywhere. And I left some
TODOs (search for TODO) for maintainers.Note: this builds upon the
lintbranch, 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.hinstead 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. ]