CMakeLists.txt for lua 5.3

[osch]

Hi, I updated CMakeLists.txt for lua 5.3 and was able to build wxLua with cmake for manjaro. I also fixed two minor issues (compiler warnings).
Best regards,
Oliver

[osch]

Addition: I added a "luaL_register emulation" that makes it possible to build with lua5.3 without the need for LUA_COMPAT_MODULE (which is not defined in default 5.3 Makefile)

[pkulchenko]

@osch, any feedback? I'd like to merge the changes, but I'd prefer the two comments to be addressed first.

[osch]

@pkulchenko, I'm sorry, but I don't understand the question. What feedback? Which comments should be addressed and how?

[pkulchenko]

I left two comments on the changes in wxLua/modules/wxlua/wxlstate.cpp; do you not see them in this ticket?

[osch]

No, I don't see any comment in wxlstate. Is this some kind of github code review feature? Did you perhaps forget to submit the review comments?

[pkulchenko]

This changed the semantics, as before lua_SetHook returned the same value as sethook call on 5.1/5.2 and nothing on 5.3. Right now it returns 1 in all cases. I don't see a need for this change, since this already takes 5.3 into account.

[osch]

a change is needed because for LUA_VERSION_NUM >= 503 the old code for the function wxLuaState::lua_SetHook does not return a value but it should because this function is declared to return int.

You could write

#if LUA_VERSION_NUM < 503
    return lua_sethook(M_WXLSTATEDATA->m_lua_State, func, mask, count);
#else
     lua_sethook(M_WXLSTATEDATA->m_lua_State, func, mask, count);
     return 1;
#endif

but this is more verbose and in fact equivalent to the code that I wrote, because lua_sethook always returns 1 for lua 5.1 & 5.2, as I deduced from the lua source code, see ldebug.c:

LUA_API int lua_sethook (lua_State *L, lua_Hook func, int mask, int count) {
  if (func == NULL || mask == 0) {  /* turn off hooks? */
    mask = 0;
    func = NULL;
  }
  L->hook = func;
  L->basehookcount = count;
  resethookcount(L);
  L->hookmask = cast_byte(mask);
  return 1;
}

or

LUA_API int lua_sethook (lua_State *L, lua_Hook func, int mask, int count) {
  if (func == NULL || mask == 0) {  /* turn off hooks? */
    mask = 0;
    func = NULL;
  }
  if (isLua(L->ci))
    L->oldpc = L->ci->u.l.savedpc;
  L->hook = func;
  L->basehookcount = count;
  resethookcount(L);
  L->hookmask = cast_byte(mask);
  return 1;
}

[osch]

Another option could be to make the method wxLuaState::lua_SetHook void.

[pkulchenko]

That's correct, but lua_sethook is void in Lua 5.3, so I think a proper fix is to restore the original logic and add condition to the return value of wxLuaState::lua_SetHook. Do you want me to do this?

[osch]

I'm sorry, but I don't understand the question nor the problem. Do you want to declare wxLuaState::lua_SetHook to return type void in the header with a preprocessor condition? This at least would solve the compiler warning. But anyway, it seems that the reuturn value is nowhere needed.So do it as you like.

[osch]

I thought again over this question: Perhaps it's best to declare wxLuaState::lua_SetHook as void in the header. So noone would get the idea to evaluate the useless return code and this is also the simplest solution. It's also good to have no compile time switch for lua version in the header, that could cause compilation errors if lua version is changed.

[pkulchenko]

I meant doing something like this:

#if LUA_VERSION_NUM < 503
int
#else
void
#endif
wxLuaState::lua_SetHook(lua_Hook func, int mask, int count)
{
     wxCHECK_MSG(Ok(), 0, wxT("Invalid wxLuaState"));
    // lua_sethook is void in 5.3
#if LUA_VERSION_NUM < 503
    return
#endif
     lua_sethook(M_WXLSTATEDATA->m_lua_State, func, mask, count);

This should address the warning and match the signature for lua_sethook, right?

[osch]

It's connected to the question what the purpose of the wxLuaState class is:

• should this class provide a one-to-one-mapping to the lua api: than there should be an #if in the header (as in your example) und you get several version of wxLuaState per proprocessor defines for different lua api versions. But in this case, there also should be an #if for luaL_register and the missing luaL_register method must be handled outside the wxLuaState class for lua5.3.
• or should the class wxLuaState be a kind of abstraction layer: than this layer should be the same for different lua versions and than there should be no #if for different lua versions. In this case there should be an luaL_register (or similar) that works the same for different lua versions and lua_sethook could have return type void for all lua versions.

[pkulchenko]

I like this refactoring, but it was luaL_openlib with Lua 5.2 before and now it's going to be luaL_register, which require COMPAT settings (and as far as I can tell, it won't compile without that setting). Not everyone compiles wxlua using the provided cmake files (I for one do not), so we can't guarantee that compatibility is turned on.

[osch]

as I deduced from the lua source code, luaL_register is just a macro for luaL_openlib in lua5.2, see lauxlib.h for lua5.2:

#if defined(LUA_COMPAT_MODULE)

LUALIB_API void (luaL_pushmodule) (lua_State *L, const char *modname,
                                   int sizehint);
LUALIB_API void (luaL_openlib) (lua_State *L, const char *libname,
                                const luaL_Reg *l, int nup);

#define luaL_register(L,n,l)	(luaL_openlib(L,(n),(l),0))

#endif

so there is no effective change for lua5.2, both versions require COMPAT settings. The difference is only for lua5.3 where the required COMPAT-5.1 settings is no longer default as it was in lua5.2

[pkulchenko]

@osch, you are right; I didn't submit the comments for review. I do see your comments as well and you should now see mine.

[osch]

@pkulchenko: Yes, you are right: now I can see your comments :-) I deleted my test comment and added comments to your comments. Best regards, Oliver

[pkulchenko]

@osch, the second change I commented on looks good, but I added a comment on the first one.

[osch]

@pkulchenko, as conclusion from our discussion I changed wxLuaState::lua_SetHook to return void. However github now lost our discussion about this topic, at least I cannot find it anymore :-(

[pkulchenko]

@osch, you need to click on "View changes" link next to the "approved these changes" message to see the changes and the related discussion. I think next time I'll put the comments in the PR itself, as I don't like that they are separate.