-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Update ccalltest #13077
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
Merged
Merged
Update ccalltest #13077
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
c440739
Move ccalltest.c to src, install into private_libdir, ensure symbols …
staticfloat ffe0d53
On OSX, libraries have .dSYM folders next to them that should be clea…
staticfloat 9cc67dc
Cull ccalltest executable as it has outlived its usefulness
staticfloat File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for other reviewers: this change shouldn't be necessary, since dllexport is the default for Mac, Linux, and Mingw64 compilers. However, it could perhaps be somewhat useful if dealing with MSVC in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
msvc can't really build this file as it stands right now since it doesn't support C-style complex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it should only really matter if msvc tried to link against the resulting file (even if mingw compiled it), which is even less likely however. and none of that matters unless you are also going to create the
lib.a
fileThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dll visibility matters for ccall - the default with mingw is only to export everything if there are no export attributes present. If you have a single dllexport that is explicitly specified, then only that function gets exported (unless you link with
-Wl,--export-all-symbols
). So it either needs to be nowhere or on everything you want to use.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the reason I added these
DLLEXPORT
things is because on Linux, with GCC, the symbols weren't accessible.nm
showed them as't'
instead of'T'
which apparently means "local", but I'm honestly a little fuzzy as to what exactly that means. It's possible that what Tony just said is the culprit; something else was getting exported which caused everything else to be non-visible, but at least things are more explicit this way.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CFLAGS has a special meaning inside of src/Makefile due to an override at the top
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point; I've removed
CFLAGS
and replaced it withCPPFLAGS
now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should still have CFLAGS as well, to accommodate where-ever the user's environment is injecting the flags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I thought that since we're setting
CFLAGS
toJCFLAGS
, we didn't want to pass them. I'll reinstateCFLAGS
alongsideCPPFLAGS
then.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it contains a lot of unnecessary cruft, but it might contain user flags