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

[lua] update to 5.3.4 #724

Merged
merged 2 commits into from
Mar 2, 2017
Merged

[lua] update to 5.3.4 #724

merged 2 commits into from
Mar 2, 2017

Conversation

codicodi
Copy link
Contributor

@codicodi codicodi commented Mar 2, 2017

Also add (off by default) option to compile as C++ and bake dllimport to headers in dynamic builds.

@ras0219-msft
Copy link
Contributor

Could you briefly summarize why you would or wouldn't use COMPILE_AS_CPP in a comment in the CMakeLists.txt? It's especially opaque to me why we don't install lua.hpp when compiling as C++ -- I'd assume it should be the opposite.

@codicodi
Copy link
Contributor Author

codicodi commented Mar 2, 2017

It is obvious once you look at its contents :)

// lua.hpp
// Lua header files for C++
// <<extern "C">> not supplied automatically because Lua also compiles as C++

extern "C" {
#include "lua.h"
#include "lualib.h"
#include "lauxlib.h"
}

As for why compile as C++ in the first place, see http://stackoverflow.com/questions/13560945/c-and-c-library-using-longjmp.
I'll add a comment about this

@ras0219-msft ras0219-msft merged commit b9b27e7 into microsoft:master Mar 2, 2017
ras0219-msft added a commit that referenced this pull request Mar 2, 2017
@ras0219-msft
Copy link
Contributor

ras0219-msft commented Mar 2, 2017

Thanks! I extended the comment a little more with some notes for users who might come after us.

In the future, it might make sense to synthesize a lua.hpp that just includes the other headers without a wrapping extern "C". This would make it possible for C++ libraries to just always include lua.hpp and work correctly regardless of whether COMPILE_AS_CPP is on or off.

@codicodi codicodi deleted the bump-lua branch March 2, 2017 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants