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

Header only library fails linking if included in >= 2 object files #51

Open
jeizenga opened this issue Jun 11, 2019 · 3 comments
Open

Comments

@jeizenga
Copy link

The gfakluge.hpp header consists mostly of methods that are neither inline nor template. That means it can't be included by multiple .cpp's, or else you will get redefinition errors at linking (because a copy of each gfakluge method gets compiled into each of the .o's). IMO, that's a pretty hefty restriction in exchange for header-only-ness.

@edawson
Copy link
Owner

edawson commented Jun 11, 2019

I see - I agree this is probably an unnecessary restriction, though I like header-only-ness a lot. I haven’t had a pattern where the hpp got included in multiple places yet.

Will inlining solve this? That’s something I’m happy to do; I don’t remember exactly why I didn’t initially but I vaguely remember it being a conscious decision.

@jeizenga
Copy link
Author

Yes, it will fix it -- I went ahead an inlined everything in the vgteam fork. If you decided not to do it before, it's probably because there's a risk of having it blow up the size of your binary. That said, compilers don't always inline when you tell them to, and they don't always not inline when you don't tell them to.

@edawson
Copy link
Owner

edawson commented Jun 12, 2019

It was probably the binary bloat issue, but I don't think I've ever seen it go too crazy in practice (plus these function just don't get written into other code repeatedly much since they're for IO). Aside: doesn't inline mean something different for classes? Who even speaks C++?

Would you mind PR'ing the changes on vgteam into here? I'll write up a tiny travis.yaml and then we can test/merge/keep the forks level.

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

No branches or pull requests

2 participants