search

rcsb / mmtf-cpp

The pure C++ implementation of the MMTF API, decoder and encoder.
MIT License
21 stars 24 forks source link

Added write and == functionality #2

Closed danpf closed 6 years ago

danpf commented 6 years ago

This was tested against the python implementation, and seems to work well. Structure Data == and print functionality is necessary for unit tests/testing and debugging.

gtauriello commented 6 years ago

Thank you very much for extending the code with a writer (and my apologies for not having done it myself in the past ;-)). One comment while glancing through the changes: was it absolutely required to use C++11 for the writer? I had kept the reader C++03 compatible to allow older compilers to use it and was hoping that the writer could remain C++03 compatible. Of course, if there's no way around it, I guess we can relax this and make it a C++11 library (the README would have to be adapted there though).

danpf commented 6 years ago

No, I think the only reason you might need c++11 is for the for (auto ...) loops.

I'm not against changing it, but c++11 is now ~7 years old, and gcc4.7 is the oldest to support it. Also c++17 is out now. Additionally, if we adopt a c++11 standard we will be able to support multithreading, which I think may be important in the future.

I think there is no reason to suspect that someone would still be using a compiler that is older than 7 years. However, I am out of touch with the real-world, is this something that happens a lot?

gtauriello commented 6 years ago

For comparison: Windows 7 is the most used operating system and it's from 2009... ;-)

And a bit more relevant: the first gcc version with full, non-experimental support of c++11 was version 4.8.1 (released in May 2013). Which yeah is still old, but I have seen gcc versions around 4.8 and older floating around here and there.

This being said: if you rely on "auto" in the library code here (I had not seen it while glancing through) I guess that's fine and we can make this c++11 only. If, on the other hand, you just need "auto" in the code which uses this library we can keep the library c++03 compliant for now. Whatever works with c++03 should also work with c++11. MsgPack for instance should happily work with both (at least it used to 1 year ago).

danpf commented 6 years ago

Nah there's no auto, it's mostly because I prefer for (int x : myvec) { and list initialization like std::vector<int>myvec{1,2,3}; so switching back to c++03 will just require changing of those lines. I will change it later today though.

danpf commented 6 years ago

This is done. Please review for any missed tests/examples/grievances if you have time :D

gtauriello commented 6 years ago

I added some minor changes that caused me trouble when testing. Since both Catch and msgpack-c are header only, there is no need to add the subfolders to cmake. Especially for msgpack-c, including the folder to cmake actually compiles the library (which adds overhead and extra dependencies).

Also I removed the old tests in examples since the Catch-unit-tests are much nicer and made them obsolete.

pwrose commented 6 years ago

Nice job Dan and thanks Gerardo for the review and refinements!

On Tue, Apr 10, 2018 at 12:26 PM, Jose Manuel Duarte < notifications@github.com> wrote:

@josemduarte approved this pull request.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/rcsb/mmtf-cpp/pull/2#pullrequestreview-110978295, or mute the thread https://github.com/notifications/unsubscribe-auth/ADuwELD-oWeRnYCId1HAw-HLtRuoS9-4ks5tnQd1gaJpZM4SSaJ7 .

gtauriello commented 6 years ago

Sorry for the delay with merging. I somehow assumed that someone else would do it during my holidays, but since there are no objections, I merged it myself now. I hope that's ok.