Error handling #13
Error handling #13
Conversation
oocairo.c
Outdated
| @@ -371,6 +371,23 @@ from_lua_rectangle (lua_State *L, cairo_rectangle_int_t *rect, int pos) { | |||
| DO(height); | |||
| #undef DO | |||
| } | |||
|
|
|||
| static int | |||
| from_lua_rectangle2 (lua_State *L, cairo_rectangle_int_t *rect, int pos) { | |||
There was a problem hiding this comment.
Is this new function necessary? I am not quite sure about the error messages that this makes possible vs the original ones, but it seems to me like your new function could just use the existing from_lua_rectangle2.
(Also: from_lua_rectangle2 is a terrible function name. How about _with_error_return? Or is that too verbose?)
There was a problem hiding this comment.
The reason for the new function is to improve error messages: if you call from_lua_rectangle instead of from_lua_rectangle2 in region_create_rectangles then you get the following error message if one of the given rectangles is invalid:
bad argument #3 to 'region_create_rectangles' (invalid rectangle)
This is confusing because argument number 3 does not make sense. With the new function this error message becomes:
bad argument #1 to 'region_create_rectangles' (list contains invalid rectangle)
I agree that the function name from_lua_rectangle2 is not the best choice. I'm trying to improve this, wait for my next commit...
There was a problem hiding this comment.
Have a look at the new function from_lua_rectangle_list_element
| assert_error(function () surface_set_mime_data(nil, "foo") end) | ||
| local ok, errmsg = pcall(function () surface:set_mime_data(nil, "foo") end) | ||
| assert_false(ok) | ||
| assert_match("bad argument %#1 to %'set_mime_data%' %(string expected%, got nil%)", errmsg) |
There was a problem hiding this comment.
Why is this a typo? The commit message confuses me.
Ah, surface_set_mime_data does not exist and thus this errored for the wrong reason. That could be made clearer in the commit message, I think...
There was a problem hiding this comment.
How could this be made clearer? The commit message explains what the commit is for. One could double the code diff in the commit message, for example "fixed typo in testcase: call surface:set_mime_data instead of undefined surface_set_mime_data". Is this better? However there are onle three lines in the commit so it should not be too hard to understand what this commit is doing.
| strcpy(copy, s); | ||
| if (copy) { | ||
| strcpy(copy, s); | ||
| } |
There was a problem hiding this comment.
Memo to myself: Get rid of my_strdup() and just use strdup() instead.
There was a problem hiding this comment.
Let us keep it as it is, because strdup is only C2x or Posix standard conform. I just experienced in another project very confusing problems using strdup (or _strdup?) under windows. Such a small function is not worth the hassle.
| if (!lua_isnumber(L, -1)) { \ | ||
| luaL_argerror(L, pos, "invalid rectangle"); \ | ||
| } \ | ||
| rect->t = lua_tointeger(L, -1); \ |
There was a problem hiding this comment.
Hm... With Lua 5.3, luaL_checkinteger does more than lua_isnumber, because 5.5 is a number, but not an integer. But lua_isinteger only exists in Lua 5.3 and not in older Lua versions.
How about something like this:
#if LUA_VERSION_NUM >= 503
#define oocairo_lua_isinteger(L, idx) lua_isinteger(L, idx)
#else
#define oocairo_lua_isinteger(L, idx) lua_isnumber(L, idx)
#endifOne could also check if the number is an integer in older Lua versions instead of silently truncating, but I am not sure if that is worth it...
There was a problem hiding this comment.
The Lua 5.1 documentation for luaL_checkinteger differs from the Lua 5.3 documentation:
Lua 5.1
luaL_checkinteger
Checks whether the function argument narg is a number and returns this number cast to a lua_Integer.
Lua 5.3
luaL_checkinteger
Checks whether the function argument arg is an integer (or can be converted to an integer) and returns this integer cast to a lua_Integer.
So if the documentation is true there was already a silent truncation of numbers to integers before my changes.
There was a problem hiding this comment.
I incorporated oocairo_lua_isinteger as you suggested, see my latest commit.
…_element": the new function has the same semantics than other functions in this area (e.g. from_lua_rectangle): a lua error is raised instead of returning a return value to indicate failure. The new function "from_lua_rectangle_list_element" correctly reports the argument number in the error message.
|
Thanks! |
fix possible memory leak (issue #8) and improve error handling
97bb961