MatteoLacki / IsoSpec

Libraries for fine isotopic structure calculator.
Other
35 stars 10 forks source link

[FEATURE] use STL container #10

Closed hroest closed 5 years ago

hroest commented 5 years ago

This is an initial PR to test the waters and see whether this kind of cleanup makes sense and will get merged by the developers. Overall, we at OpenMS are a bit concerned about the manual memory management with new and delete and would like to change this in the code but we would like to do it with upstream support.

Is there a strong need to do manual memory management or a reason why STL containers cannot be used in the code?

michalsta commented 5 years ago

There were some reasons, like close-to-the-bare-metal efficiency for tons of really tiny molecules (in which case the runtime is dominated by the setup of various Iso/Marginal classes, and using STL would probably incur some small but nonzero overhead). Still, if you really are dead set on eliminating new/delete's from OpenMS, I think that keeping the codebases in sync as far as possible trumps that concern, so feel free to go ahead, and I'll merge the whole thing once you're done. Just please, test the heck out of it ;)

hroest commented 5 years ago

maybe @cbielow has some comments here

Personally, I am not dead set on this and in our testing there have been no memory leaks in the code. So I think that since it is working right now, and since I dont have a deep understanding of the code, I would keep things as they are.