Always build a cdecl library #4930
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
TortoiseGit always have been using |
Awesome, thanks for the update. That was my assumption but I appreciate the confirmation. |
|
I know that nodegit itself does not specify stdcall; however, it is /possible/ (though I doubt it) that the node build system (node-gyp) could be defining stdcall. I've been in meetings for 2 days, but I will get back to you after I have checked. |
|
We're using |
|
OK, I've let a few people who are doing FFI and interop on Windows (LibGit2Sharp, etc) and so I think that most people that we know about haven't had any complaints. Holding on to this just so that somebody can have an opportunity to argue with me about the |
The GIT_EXTERN macro needs to provide order-specific attributes; update users of the GIT_DEPRECATED macro to allow for that.
The recommendation from engineers within Microsoft is that libraries should have a calling convention specified in the public API, and that calling convention should be cdecl unless there are strong reasons to use a different calling convention. We previously offered end-users the choice between cdecl and stdcall calling conventions. We did this for presumed wider compatibility: most Windows applications will use cdecl, but C# and PInvoke default to stdcall for WINAPI compatibility. (On Windows, the standard library functions are are stdcall so PInvoke also defaults to stdcall.) However, C# and PInvoke can easily call cdecl APIs by specifying an annotation. Thus, we will explicitly declare ourselves cdecl and remove the option to build as stdcall.
To explicitly break end-users who were specifying STDCALL, explicitly fail the cmake process to ensure that they know that they need to change their bindings. Otherwise, we would quietly ignore their option and the resulting cdecl library would produced undefined behavior.
Since we now always build the library with cdecl calling conventions, our callbacks should be decorated as such so that users will not be able to provide callbacks defined with other calling conventions. The `GIT_CALLBACK` macro will inject the `__cdecl` attribute as appropriate.
ethomson
force-pushed
the
ethomson/cdecl
branch
from
1ffb167
to
38e6179
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On Windows, we currently offer end-users the ability to compile the library with cdecl calling conventions (the default) or stdcall calling conventions (an option enabled by
-DSTDCALL=ONto cmake).The recommendation from engineers within Microsoft is that libraries should have a calling convention specified in the public API, and that calling convention should be cdecl unless there are strong reasons to use a different calling convention.
We offered stdcall for the LibGit2Sharp project, since by default, PInvoke (the .NET FFI) uses stdcall. This is because PInvoke is usually used to call standard library functions, and Win32 is stdcall for historic reasons. But the defaults for all new projects is to use cdecl, and most other modern libraries are using it.
(In either case, we should pick one and declare it in our API. Without having done so, the end user must match our calling conventions in order to even operate. This is onerous, and we should allow interop to programs from any calling convention.)
This PR removes the
STDCALLoption from cmake, and adds the necessarycdeclattributes toGIT_EXTERN. In addition, we need to identify the calling conventions for our callbacks, so I've introduced theGIT_CALLBACKmacro to do so and updated our public callbacks to have that attribute.Note that this is a breaking change for anybody who was building with
STDCALL=ON. However I suspect that LibGit2Sharp is the only consumer who's done that, since it made their bindings somewhat easier. But it's not impossible that there's somebody building with stdcall who will need to update their callbacks to be cdecl (one simply needs to add the attribute).So this is a breaking API change, but I suspect it will not affect too many end users. But @csware and @implausible are probably the most likely to be affected and I'd like their input.
I have a PR for LibGit2Sharp ready to go to move over to cdecl, to ensure that it will work (it does).