search

edawson / gfakluge

A C++ library and utilities for manipulating the Graphical Fragment Assembly format.
http://edawson.github.io/gfakluge/
MIT License
51 stars 20 forks source link

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

Open jeizenga opened 5 years ago

jeizenga commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.