Add macros to set calling conventions for callbacks. #904
Conversation
Required to successfully link when compiling with non-default calling conventions.
There was a problem hiding this comment.
I'm not sure how /Gv works, when you say:
they will not be able to call imgui functions
Do you mean none of the imgui functions can be called? or is just that your functions with default settings won't work as imgui as callbacks?
Wouldn't it saner and simplier if your app defined the few callbacks it needs for imgui as __cdecl?
| #else | ||
| #define IMGUI_CALLBACK | ||
| #endif | ||
|
|
There was a problem hiding this comment.
Confused about how this blocks work, wouldn't statement in the else override whatever the user has specified?
There was a problem hiding this comment.
empty define result in replacement with nothing.
aka static const char* IMGUI_CALLBACK GetClipboardTextFn_DefaultImpl(void* user_data); turns into static const char* GetClipboardTextFn_DefaultImpl(void* user_data); letting the default calling conv take over.
There was a problem hiding this comment.
Yes but isn't the point that the user might want to define IMGUI_CALLBACK to something that's not your default?
Shouldn't the correct test should just be:
#ifndef IMGUI_CALLBACK
#define IMGUI_CALLBACK
#endif
Let the user do what they want and not involve _WIN32 there?
There was a problem hiding this comment.
If you're on _WIN32, __cdecl is the correct default. It's the Windows standard callback ABI that is used by all callbacks in Microsoft's headers (they have a CALLBACK macro that does the same thing here). If you decide to use a different ABI for callbacks on Windows, several of the default callbacks in imgui will become illegal.
This whole thing is basically a Windows-ism, so it really does make since to only do this for _WIN32, and even supporting non-default ABIs like we do is more of a super advanced need that could probably just be removed.
A large and ever better fix here would be for imgui to follow the standard Windows practice of using a macro to declare the ABI for every non-inline function so that imgui is not affected by default ABI settings. That would allow imgui and the host app to be compiled with different defaults and still have everything link and run. That is not essential to fixing the bug this PR is address, though.
In the case that the host app is compiled with If you compile imgui with Since tossing |
|
Hello @seanmiddleditch, and my apologies for keeping that PR dangling for so long. I recently merged in #1611 which seems to tackle (partly) the same issue but only changed the 3 functions (there was 2 at the time of your PR) that are called by e.g. libc I am a little confused however about the extra changes affecting e.g. |
|
Hi, yeah it would affect things if calling connections differ. Not my use case but still a potentially valid one to consider. |
|
Hello Sean, |
|
@ocornut - ah, don't think I did that intentionally, and never got through my github notifications backlog until now to see this question :( -- cool to reopen? |
|
Sure! |
|
Cool, thanks. It's been a few years, but... are you on board with the general gist of fixes? I can get this updated to the latest master. Since you're on Github Actions now I can also add a test specifically for the case I ran into with using |
0c1e5bd to
bb6a60b
Compare
8b83e0a to
d735066
Compare
b3b85d8 to
0755767
Compare
c817acb to
8d39063
Compare
Required to successfully link when compiling with non-default calling conventions.
e.g., if the hosting app is compiled with /Gv to use the __vectorcall calling convention by default, which is sometimes a huge performance improvement, they will not be able to call imgui functions. Compiling imgui itself with /Gv does not work since system callbacks also need to be marked.
This change makes a single universal IMGUI_CALLBACK macro for marking up all callbacks, typically to __cdecl. If there is a desire to allow imgui callbacks to have custom calling conventions (if there is ever a high-volume callback, for instance) then there may need to be separate macros for CRT callbacks and imgui callbacks.
This change also modified an stb header that uses
qsortand hence needs to set calling convention. That change could be pushed back to stb if there is a desire to avoid patches over stb.