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

Make __declspec(dllimport) optional (and off by default) #45

Closed
wants to merge 2 commits into from

Conversation

roothorick
Copy link

The added comment sums up the rationale, but to elaborate... the two big problems avoided by omitting it:

  • dllimport is not supported by GCC on non-Windows hosts, regardless of the target platform. In other words, when cross-compiling to Windows, dllimport is unavailable and will generate a compile-time error.
  • The compiler takes this prefix as a hint that it can assume the method will be provided by a DLL, and therefore generates a direct call into the DLL. This doesn't fly if the library is actually being statically linked -- something the compiler can't know ahead of time.

Of course, there may be edge cases where not prefixing with dllimport will cause undesired behavior, or generate a more substantial relocation thunk with a significant performance impact. Those that find themselves dealing with such cases can simply define EPOXY_USE_DLLIMPORT where necessary.

This is a pretty good explanation of what __declspec(dllimport) actually does: http://blogs.msdn.com/b/russellk/archive/2005/03/20/399465.aspx?wa=wsignin1.0

@endrift
Copy link

endrift commented Nov 2, 2015

I had to make a similar change to this when I made a static build for Windows. I'm not sure this approach is the best, but it's certainly better than the nothing this library has at the moment.

@yaronct
Copy link
Contributor

yaronct commented Nov 2, 2015

@roothorick, @endrift: U may want to check my fork where I believe I've fixed this issue. It also supports building with CMake, including static builds, and several bug fixes.

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