-
Notifications
You must be signed in to change notification settings - Fork 394
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
Prevent extern "C"
propagation
#933
base: master
Are you sure you want to change the base?
Conversation
`extern "C"` is C++ specific and if we enable it our consumers are likely not C++
@@ -137,14 +137,14 @@ if(LUAU_EXTERN_C) | |||
# enable extern "C" for VM (lua.h, lualib.h) and Compiler (luacode.h) to make Luau friendlier to use from non-C++ languages | |||
# note that we enable LUA_USE_LONGJMP=1 as well; otherwise functions like luaL_error will throw C++ exceptions, which can't be done from extern "C" functions | |||
target_compile_definitions(Luau.VM PUBLIC LUA_USE_LONGJMP=1) |
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 was unsure about if this should also be made private.
There doesn't appear to be anything in the header that makes use of it and a consumer may want to know about this.
target_compile_definitions(Luau.VM PUBLIC LUA_API=extern\"C\") | ||
target_compile_definitions(Luau.Compiler PUBLIC LUACODE_API=extern\"C\") | ||
target_compile_definitions(Luau.VM PRIVATE LUA_API=extern\"C\") | ||
target_compile_definitions(Luau.Compiler PRIVATE LUACODE_API=extern\"C\") | ||
endif() | ||
|
||
if(LUAU_NATIVE) | ||
target_compile_definitions(Luau.VM PUBLIC LUA_CUSTOM_EXECUTION=1) |
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.
Same as the other
Sorry for the long response, we had to find someone who knows how CMake works. This change will result in anything downstream from The better solution would have been to use a generator expression like |
The generator expressions for the language appears to have been introduced with cmake 3.3 not 3.15, but that is still above the minimum version luau requires. I don't think a single I'm still looking around for other alternatives, but a massive workaround would be setting |
This change breaks exporting to a static and dynamic library which breaks embedding Luau in e.g CSharp. This is not a useful change and there should be specific settings added for exporting to a DLL, static library and basic |
As far as I can tell the cmake setup does not currently support building a dyanamic library, so this appears to a moot point, though it does affect how Luau would be consumed by C++ projects, breaking compatibility there.
This change needs to be done somehow regardless. Most languages rely on the C ABI for their FFI and those that can consume the header files (e.g. C, Zig, etc.) may not understand An alternative (that I think would be best overall) would be dropping the build option and forcing extern C on the entire header when C++ is used. |
@zeux I wonder if we should always use "C" linkage without relying on CMake and place them in a block like: #ifndef __cplusplus
extern "C" {
#endif |
Unfortunately we can't use It does seem like we'd need to have a separate LUA_EXTERN_C define :-/ I think the DLL export/import can still be handled as we do now (with CMake configuration without explicit support)? |
I ran into this when trying to use Luau from a project written in C. When In case it helps anyone, I have the following workaround which manually rips out the foreach(target Luau.VM Luau.Compiler)
get_target_property(defs ${target} INTERFACE_COMPILE_DEFINITIONS)
list(FILTER defs EXCLUDE REGEX [[LUA.*_API]])
set_target_properties(${target} PROPERTIES INTERFACE_COMPILE_DEFINITIONS "${defs}")
endforeach()
# target_link_libraries(myproject Luau.VM Luau.Compiler Luau.Ast) etc. This allows me to use luau as-is without having to apply this change on a fork. |
This snippet fixed my issue just now when trying to do the exact same thing; Luau in a C project. Thanks! |
extern "C"
is C++ specific and if we enable it our consumers are likely not C++.