Conversation
…dentified by Clang and Clang++. They are all minor. The result passes 'ctest -L long' but pyEXP-example notebooks have not been run.
michael-petersen
left a comment
michael-petersen
left a comment
There was a problem hiding this comment.
Very explicit definitions now, I like it!
Not sure how this will play with pybind11fix. I'm tempted to try merging this into pybind11fix first?
| virtual void get_pot_dpot(const double, double&, double&) = 0; | ||
|
|
||
| double get_mass(const double x1, const double x2, const double x3) | ||
| double get_mass(const double x1, const double x2, const double x3) override |
There was a problem hiding this comment.
Nice -- I didn't know about override before. Definitely reduces the warnings!
utils/ICs/gensph.cc
Outdated
| double a[NUMR], b[NUMR], c[NUMR]; | ||
| fftw_complex A[NUMR], B[NUMR], C[NUMR]; | ||
| std::vector<double> a(NUMR), b(NUMR), c(NUMR); | ||
| std::vector<fftw_complex> A(NUMR), B(NUMR), C(NUMR); |
There was a problem hiding this comment.
This line was causing problems for me:
error: object expression of non-scalar type 'double[2]' cannot be used in a pseudo-destructor expression
I think this is because typedef double fftw_complex[2]; is the under-the-hood definition in FFTW, which is causing a problem when passed to std::vector as MacOS handles it.
There was a problem hiding this comment.
This can be handled using a std::unique_ptr<fftw_complex> instead. But a bit surprised that Mac and Linux would be different here.
There was a problem hiding this comment.
From RTFM, fftw knows about C complex, so I think we can get rid of fftw_complex in favor of std::complex which is compatible with C complex. That is, std::vector<std::complex<double>> rather than std::vector<fftw_complex>. I'll try it now...
There was a problem hiding this comment.
Given that this passes CI, it seems unlikely it will cause trouble for anyone else. I think it's appropriate to try a merge of this PR after someone else checks that these changes make sense.
|
Now also addressing #53. |
|
This branch has |
|
Please try the latest refactoring on your Mac, when you have a chance. TL;DR: I agree with the use of |
michael-petersen
left a comment
michael-petersen
left a comment
There was a problem hiding this comment.
This checks out for me, thanks!
|
All |
2 participants
Summary
This addresses all the warnings generated by
clang++from PR #85 and includes that PR.There are three types of fixes:
std::vector<T>or astd::unique_ptr<T[]>to automatically manage memory.std::shared_ptrto do a dynamic cast for a type was flagged by the compiler as "expressions with side-effects", even though I think they are probably okay . These were switched to dynamic casts to pointers to types, with no "side effects".The compiled code passes
ctest -L longtests. Several of these changes are inexpuicode. While I do not think that the likelihood of breaking code is low (since most of these changes are in memory management for buffers), I might be good to check this code against thepyEXP-examples/Tutorialsnotebooks for verification.