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

Use std::shared_ptr instead of our own RC* function. #399

Merged
merged 26 commits into from Dec 23, 2019
Merged

Conversation

papadop
Copy link
Contributor

@papadop papadop commented Dec 14, 2019

Replaces PR #396 (that I have not been able to rebase)....
This PR removes the use of our own shared pointer solution in favor of the standard std::shared_ptr. Hopefully it will also help to improve the python binding as the std::shared_ptr is already handled by swig. This also remove some files which we no longer have to maintain and simplifies a little bit the code. Tested on fedora 31, let's see what the other platforms will say....

@codecov
Copy link

codecov bot commented Dec 16, 2019

Codecov Report

Merging #399 into master will decrease coverage by 0.02%.
The diff coverage is 78.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #399      +/-   ##
==========================================
- Coverage   70.09%   70.07%   -0.03%     
==========================================
  Files          96       95       -1     
  Lines        6604     6576      -28     
==========================================
- Hits         4629     4608      -21     
+ Misses       1975     1968       -7
Impacted Files Coverage Δ
OpenMEEGMaths/src/symmatrix.cpp 59.59% <0%> (ø) ⬆️
OpenMEEGMaths/src/matrix.cpp 76.71% <100%> (ø) ⬆️
OpenMEEGMaths/include/linop.h 92.85% <71.42%> (+12.85%) ⬆️
OpenMEEGMaths/include/vector.h 57.4% <75%> (ø) ⬆️
OpenMEEGMaths/include/symmatrix.h 45.83% <85.71%> (ø) ⬆️
OpenMEEGMaths/include/matrix.h 77.77% <87.5%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 749fd51...c24aa1c. Read the comment docs.

@papadop
Copy link
Contributor Author

papadop commented Dec 18, 2019

It looks like this is an heisenbug in travis. 2 failures previously (linux), one now (mac which was passing previously, and the change is a dummy one - removal of a comment -). So I think this is ready to be merged...

Please do not squash the merge, as it makes further merges difficult (this was done with the previous PR, which obliged me to do a manual merge with all the time and the risks this incurs).

@agramfort
Copy link
Contributor

@papadop you've been missing my comments or just ignoring them but I am not ok with adding a dependency on boost and I don't want to merge code that makes CIs red.

sorry for being opinionated. I find this other PR #397 more problematic and urgent as it reveals an actual bug that prevents me from moving forward with mne-openmeeg integration

@papadop
Copy link
Contributor Author

papadop commented Dec 18, 2019

Sorry but I have not seen your comment... not on this PR at least.
The depedency on boost is only a compile time dependency and only for windows, which does not have this standard header, so in my view it is not a problem from the user point of view. Note also that boost is pre-installed on appveyor, so that I did not even had to change the .yml...

As for the red, read my comment. It is a problem with travis, not with the code. Two successive runs (with no change) result in two different results... If travis is not reliable, then we will have to re-consider this policy.... Hopefully it is just a temporary problem.

@papadop
Copy link
Contributor Author

papadop commented Dec 18, 2019

As for PR #397, I'm on it... But the solution is more complex than you may think. The problem is hidden in the mess of our mesh/geometry structure and a probable solution is .... shared_pointer !!!
PR 397 is because the vertices obtained from the python construction are duplicated... So the nested domains are not detected properly. I think I explained this in the PR.

I spent most of my week-end in trying to clean the mesh/geometry code to the point it will be more manageable.... Yet another can of worms I opened !!! So I'm on it, but it evolves as fast as I have time to deal with it.

@papadop
Copy link
Contributor Author

papadop commented Dec 18, 2019

Travis is green ;-)
Thank's for the tip allowing to restart only the failed job !!!

@papadop papadop merged commit 9d32be5 into master Dec 23, 2019
@papadop papadop deleted the NewSharedPtr branch December 23, 2019 13:49
# CHECK_CXX_FEATURE(HAVE_MATH_FN_IN_NAMESPACE_STD math_fn_in_namespace_std.cpp "has C math functions in namespace std")
# CHECK_CXX_FEATURE(HAVE_MATH_ABSINT_IN_NAMESPACE_STD math_absint_in_namespace_std.cpp "has C math abs(integer type) in namespace std")
# CHECK_CXX_FEATURE(HAVE_COMPLEX_MATH_IN_NAMESPACE_STD complex_math_in_namespace_std.cpp "supports complex math functions are in namespace std")
# CHECK_CXX_FEATURE(HAVE_ISNAN_IN_NAMESPACE_STD isnan_in_namespace_std.cpp "has isnan function in namespace std")
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI @papadop these had been commented out as it was not used and it was increasing cmake bulid time for nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes pre-approved. Maybe you can integrate it with your old CMake removal. I already have merged this branch....

Copy link
Contributor

Choose a reason for hiding this comment

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

see #403

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants