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

Move implementation into cpp #12

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

psyklopz
Copy link

Addresses issue #9

When Configuru is included in separate translation units, it can result in LNK2005 errors on MSVC.
GCC seems to handle this fine, but it is still a violation of the One Definition Rule, where it appears Configuru is still built into multiple translation units.

This commit is similar to that done on Loguru two years ago.

I have shown this works successfully in two different projects that utilize Configuru in separate units. One uses two different configuration files. The other uses Configuru for a config file and then some simple JSON formatting for a web API.

I don't use Boost and really didn't feel like messing with getting that to work. So I was able to build the test suite (using std::experimental::filesystem instead of boost::filesystem) and confirm the tests still all pass. I've not included this in the merge request, but if interested the code that works with std::experimental::filesystem can be seen on the separate_into_hpp_and_cpp branch of psyklopz/Configuru.

Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

I think this is an excellent idea, but we no longer need CONFIGURU_IMPLEMENTATION. In fact, its use is indicative of a user error - someone updated Configuru and did not realize they now need to make use of configuru.cpp.

#include <configuru.hpp>
Then, in only one .cpp file:
#define CONFIGURU_IMPLEMENTATION 1
#include <configuru.cpp>

For more info, please see README.md (at www.github.com/emilk/configuru).
*/
Copy link
Owner

Choose a reason for hiding this comment

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

Here it would be good with this check:

#if defined(CONFIGURU_IMPLEMENTATION)
    #error "You are defining CONFIGURU_IMPLEMENTATION. This is for older versions of Configuru. You should now instead include configuru.cpp (or build it and link with it)"
#endif


And in one .cpp file:
Then, in only one .cpp file:

``` C++
#define CONFIGURU_IMPLEMENTATION 1
Copy link
Owner

Choose a reason for hiding this comment

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

We do not need CONFIGURU_IMPLEMENTATION anymore!

@psyklopz
Copy link
Author

I will make these changes. I'm a little swamped at the moment, so it probably won't be until next week.

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.

3 participants