httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Rici Lake" <r...@ricilake.net>
Subject Re: [PATCH] mod_wombat: add table_get and table_set
Date Fri, 04 May 2007 23:42:26 GMT
A few comments intermingled into the patch:

Brian McCallister wrote:
> On Apr 30, 2007, at 2:02 PM, Akins, Brian wrote:
> --- apr_lua.c	(revision 0)
> +++ apr_lua.c	(revision 0)
> @@ -0,0 +1,55 @@
> +#include "apr.h"
> +#include "apr_tables.h"
> +
> +#include "lua.h"
> +#include "lauxlib.h"
> +#include "lualib.h"
> +
> +#define lua_unboxpointer(L,i)      (*(void **)(lua_touserdata(L, i)))
> +
> +static apr_table_t* check_apr_table(lua_State* L, int index) {
> +    luaL_checkudata(L, index, "Apr.Table");
> +    apr_table_t* t = (apr_table_t*)lua_unboxpointer(L, index);
> +    return t;
> +}
> +
> +static int lua_table_set(lua_State* L) {
> +    apr_table_t *t = check_apr_table(L, 1);
> +    const char* key = luaL_checkstring(L, 2);
> +    const char* val = luaL_checkstring(L, 3);
> +
> +    apr_table_set(t, key, val);

In Lua, setting to nil is equivalent to deletion. So I think this should be:

  if (lua_isnoneornil(L, 3)
    apr_table_unset(t, key);
  else {
    const char *val = luaL_checkstring(L, 3);
    apr_table_set(t, key, val);
  }
> +    return 0;
> +}
> +
> +static int lua_table_get(lua_State* L) {
> +    apr_table_t *t = check_apr_table(L, 1);
> +    const char* key = luaL_checkstring(L, 2);
> +    const char *val = apr_table_get(t, key);

val might be NULL; the function should return nil in that case:

  if (val == NULL)
    lua_pushnil(L);
  else
    lua_pushstring(L, val);

> +    lua_pushstring(L, val);
> +    return 1;
> +}
> +
> +static const luaL_reg lua_table_methods[] = {
> +    {"set", lua_table_set},
> +    {"get", lua_table_get},
> +    {0, 0}
> +};
>
> Even though these are static, we might want to be careful in naming
> as these are reaching into lua's namespace (lua_* and luaL_*).

Agreed. Also, it's misleading -- they are APR tables, not Lua tables.

>
> +
> +
> +int apr_lua_init(lua_State *L, apr_pool_t *p) {
> +    luaL_newmetatable(L, "Apr.Table");
> +    luaL_openlib(L, "apr_table", lua_table_methods, 0);
> +    lua_pushstring(L, "__index");
> +    lua_pushstring(L, "get");
> +    lua_gettable(L, 2);
> +    lua_settable(L, 1);
> +
> +    lua_pushstring(L, "__newindex");
> +    lua_pushstring(L, "set");
> +    lua_gettable(L, 2);
> +    lua_settable(L, 1);
> +
> +    return 0;
> +}
>

This is poor Lua style. You shouldn't assume (or require) the stack to
be empty at the start of the function. Use top-relative (negative) indices
instead -- they are no slower.

Also use lua_{get,set}field for clarity, and luaL_register (luaL_openlib
is deprecated).

  luaL_register(L, "apr_table", lua_table_methods);
  luaL_newmetatable(L, "Apr.Table");
  lua_getfield(L, -2, "get");
  lua_setfield(L, -2, "__index");
  lua_getfield(L, -2, "set");
  lua_setfield(L, -2, "__newindex");





Mime
View raw message