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

Added write and == functionality #2

Merged
merged 29 commits into from
May 15, 2018
Merged

Added write and == functionality #2

merged 29 commits into from
May 15, 2018

Conversation

danpf
Copy link
Collaborator

@danpf danpf commented Feb 25, 2018

  • Updated all example data to the newest format
  • Updated compile_target to use c++11
  • Made encoder functional
  • Added == and print functionality to structure_data
  • Fixed a few small bugs
  • Added example code for writing to file

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.

  • Add back c++0x compatibility
  • Added submodules for catch and msgpack-c
  • Add unit tests
  • Update README
  • Remove examples data (in favor of mmtf-spec data)

* Updated all example data to the newest format
* Updated compile_target to use c++11
* Made encoder functional
* Added == and print functionality to structure_data
* Fixed a few small bugs
* Added example code for writing to file

This was tested against the python implementation, and seems to work
well.  == and print functionality is necessary for unit tests/testing
and debugging.
@gtauriello
Copy link
Collaborator

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
Copy link
Collaborator Author

danpf commented Feb 26, 2018

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
Copy link
Collaborator

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
Copy link
Collaborator Author

danpf commented Feb 26, 2018

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
Copy link
Collaborator Author

danpf commented Apr 3, 2018

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

@gtauriello
Copy link
Collaborator

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.

Copy link
Collaborator

@gtauriello gtauriello left a comment

Choose a reason for hiding this comment

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

Thanks for the great work Dan.
The new code works and as a bonus there's cmake files and proper unit tests.
From my point of view, this can be merged. Any objections?

@pwrose
Copy link
Collaborator

pwrose commented Apr 10, 2018 via email

@gtauriello gtauriello merged commit b37bdc9 into rcsb:master May 15, 2018
@gtauriello
Copy link
Collaborator

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.

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.

4 participants