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

Distinct SOKOL_API macros for libs #428

Closed
iboB opened this issue Nov 18, 2020 · 10 comments
Closed

Distinct SOKOL_API macros for libs #428

iboB opened this issue Nov 18, 2020 · 10 comments

Comments

@iboB
Copy link
Contributor

iboB commented Nov 18, 2020

This is a Windows-only feature request, as only Windows compilers have special export/import symbols for DLLs.

On Windows it's not possible to put different sokol libs in different DLLs which reference each other, as all libs use the same SOKOL_API_DECL/IMPL export macros.

My particular use case is putting imgui along with sokol_imgui in a separate dll because not all modules use/need imgui. In that dll I want sokol_imgui functions to be dllexport and other sokol functions to be dllimport.

To keep backwards compatibility this can be used:

// sokol_gfx.h
// assuming SOKOL_GFX_API is the macro for sokol_gfx
#if defined(SOKOL_API_DECL)
    #define SOKOL_GFX_API_DECL SOKOL_API_DECL
#endif

#if !defined(SOKOL_GFX_API_DECL) 
// ... 
// same defines and checks as the the ones which used to be for SOKOL_API_DECL

If the proposal is approved, I can whip up PRs for each library

PS I realize that a workaround by creative including is possible: define-include-undef-define-include, but this won't work with C++20 modules, and also it's pretty unpleasant code to write and maintain...

@floooh
Copy link
Owner

floooh commented Nov 18, 2020

Yeah, this sounds like a good idea. PRs would be welcome.

Somewhat related, I've been thinking for quite a while that this would also make sense for the SOKOL_IMPL define (for the utility headers I'm already doing this). But I'll take care of this at a later time.

@meshula
Copy link

meshula commented Nov 18, 2020

Here is the pattern I use in my projects. Demonstrating using USD, each library has its own set of macros - for example:

https://github.com/PixarAnimationStudios/USD/blob/release/pxr/base/tf/api.h

Which pulls in the base definitions:

https://github.com/PixarAnimationStudios/USD/blob/release/pxr/base/arch/export.h

That last file explains pretty clearly the reasoning behind the structure. I've used many different variations over the years, and this feels like the "boss monster final form" version.

@iboB
Copy link
Contributor Author

iboB commented Nov 18, 2020

@floooh, great!

Do you prefer separate PRs for each lib or one PR for all libs?

@floooh
Copy link
Owner

floooh commented Nov 19, 2020

Do you prefer separate PRs for each lib or one PR for all libs?

Just put everything into one PR, that's less noisy :)

@floooh
Copy link
Owner

floooh commented Nov 19, 2020

PS: I've been thinking a little bit about how to handle SOKOL_IMPL in the future, and I like the idea to allow SOKOL_IMPL as a "catch-all" define if one doesn't need per-library defines, but still allow SOKOL_GFX_IMPL, SOKOL_APP_IMPL etc...

Each header would then simply have a:

#if defined(SOKOL_IMPL)
#define SOKOL_GFX_IMPL
#endif

...to make everything work (and that's why I think we should also keep SOKOL_API_DECL and SOKOL_API_IMPL for the future and not just as a backward compatibility hack).

@iboB
Copy link
Contributor Author

iboB commented Nov 19, 2020

Definitely, yeah.

I was thinking of adding:

#if defined(SOKOL_API_DECL) && !defined(SOKOL_GFX_API_DECL)
// ...

thus a user can use the catch-all export macros and special-case certain libs if they choose

@iboB
Copy link
Contributor Author

iboB commented Nov 19, 2020

btw, do you mind a draft PR? The problem with it is that you'll get notifications for my commits before the PR is ready to merge

@floooh
Copy link
Owner

floooh commented Nov 19, 2020

btw, do you mind a draft PR

No problem, do as you like :)

iboB added a commit to iboB/sokol that referenced this issue Nov 19, 2020
iboB added a commit to iboB/sokol that referenced this issue Nov 19, 2020
@iboB
Copy link
Contributor Author

iboB commented Nov 24, 2020

@floooh not sure if you noticed, but my I think PR - #429 - is ready to merge. Do you think it needs anything more?

@floooh
Copy link
Owner

floooh commented Nov 24, 2020

Yep, I noticed :) I'll try to take care of it within the next few days. I'm trying some sort of "merge day" approach where I try to focus on pending PRs and nothing else, otherwise it's too distracting.

@floooh floooh closed this as completed in 426c460 Nov 28, 2020
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

3 participants