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

Export defines PUBLIC so that headers work properly #250

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xsacha
Copy link

@xsacha xsacha commented Mar 13, 2019

COMMON_TARGET_DEFINITIONS needs to be PUBLIC on the built libraries so that it is exported to the sleefConfig.cmake.

Otherwise, the sleef.h header does not work properly.
SLEEF_STATIC_LIBS and ENABLEFLOAT128 are two examples that need to be exported. I am unsure if all defines need to be exported but currently the defines are not distinguished in any way.

)

if(COMPILER_SUPPORTS_FLOAT128)
# TODO: Not supported for LLVM bitcode gen as it has a specific compilation flags
target_sources(${TARGET_LIBSLEEF} PRIVATE sleefqp.c)
target_compile_definitions(${TARGET_LIBSLEEF}
PRIVATE ENABLEFLOAT128=1 ${COMMON_TARGET_DEFINITIONS})
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

COMMON_TARGET_DEFINITIONS was removed from this line because it is already added in the previous target_compile_definitions

@fpetrogalli
Copy link
Collaborator

Hi @xsacha , could you please explain what are you trying to fix here? You say that "sleef.h doesn't work properly". What do you mean by that? What is this patch allowing you to do with sleef.h that was not possible to do before?

@xsacha
Copy link
Author

xsacha commented Apr 26, 2019

What I mean is that if you include the header, it will not have the same defines that were used at compile time.

@fpetrogalli
Copy link
Collaborator

if you include the header, it will not have the same defines that were used at compile time

Hi , thank you for explaining, but this patch is still not cleat to me.
Why would sleef.h need to have the same defines that are used at build time? Becasue from what you are saying, I understand that you are thinking of having those defines in sleef.h, did I get it right? If that's the case, could you provide and example when this is needed?

Apologies for asking you to explain more, but I am not a cmake expert and I really don't see why we should export those defines.

@xsacha
Copy link
Author

xsacha commented Apr 29, 2019

Any defines provided through the CMake are not sent when linking the project unless you use the correct CMake functions.

PUBLIC is INTERFACE + PRIVATE combined.
By default these are only private (used as compile time but not for anything that links it).
By making it PUBLIC, anyone who externally links to the project will also get the defines.

@fpetrogalli
Copy link
Collaborator

By making it PUBLIC, anyone who externally links to the project will also get the defines.

Does this mean that you need to gives visibility to those defines to other CMake projects that include SLEEF as a subproject? Is this what you mean by "externally link"? All these defines are doing is just selecting how to recompile the sources, why would an external project need to see them?

@xsacha
Copy link
Author

xsacha commented Apr 29, 2019

Because when I tried to use sleef headers, it threw undefined linker errors because variables such as avx and float128 were defined at compile time but not when I linked to it in CMake.

This commit fixed these errors.

@shibatch
Copy link
Owner

@fpetrogalli How about this patch? I think this patch is safe to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants