search

rcsb / mmtf-cpp

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

Fix for inclusion in multiple CPP files #3

Closed frodofine closed 6 years ago

frodofine commented 6 years ago

Attempting to include structure_data.hpp in more than one translation unit results in linkage errors as three functions are defined multiple times. See this article for details.

gtauriello commented 6 years ago

Thanks for catching that. I thought I had checked for those things... ;-)

I just noticed though that the same should happen for the functions of the BinaryDecoder and MapDecoder classes. Probably it depends on what code you use (BinaryDecoder and MapDecoder only get used via templated functions which the compiler may treat differently), but still: it's probably cleaner to be consistent in all headers. Could you add the inlines for those classes too?

Also do you happen to have a mini example with two compilation units? Otherwise, I can cook one up tomorrow at work. Just curious if this is compiler dependent...

frodofine commented 6 years ago

I've tested it on GCC 5.2.0 and MSVC 15, both result in linkage errors. The inclusion of that specific header file caused the issues, but I did not test BinaryDecoder and MapDecoder. I'm not sure what the standard says about templated functions being inlined by default, but I would imagine that they would be because otherwise, things would get messy fast.

I do not have an example ready, but I can come up with one fairly easily.

gtauriello commented 6 years ago

I just added an automated test to the cmake in mmtf-cpp/tests and noticed that I needed more inlines for this to compile with GCC 4.8.4. Unfortunately I couldn't push my changes to your fork. Can you add me as collaborator (as in https://help.github.com/articles/inviting-collaborators-to-a-personal-repository/) so that I can push it?

I somehow assumed I would have permission to push there anyways and forking your fork seemed overkill... ;-)

frodofine commented 6 years ago

Sure thing, I've added you.

gtauriello commented 6 years ago

Thanks. I pushed my changes. Can you check if the cmake setup works for you too? Basically, the cmake command described in the main README should trigger the compilation of a mini-test which links objects from two cpp files...

frodofine commented 6 years ago

The tests work for me!

frodofine commented 6 years ago

No problems for this PR, I have another PR almost ready that fixes many issues on Windows.

gtauriello commented 6 years ago

I was more checking if anyone from RCSB had objections... ;-) I will merge and close the PR tomorrow or Friday. Thanks again for your contributions.

josemduarte commented 6 years ago

Looks good to me @gtauriello . Please go ahead