SVCD::notify [NOT READY]

[samkumar]

Leonard, Michael, and I have implemented SVCD.notify in C. Please give us suggestions and let us know if you find any bugs.

Should we also push our tests to GitHub?

[leonardt]

we could use

int arr_as_str(lua_State *L);

[mccormickt12]

is lua_pushnumber(L, arr_type_uint16) the correct code. I did the same thing but couldn't find out why its correct?

[samkumar]

lua_pushnumber(L, ARR_TYPE_UINT16)

is the correct code. In Lua you would do

storm.array.create(1, storm.array.UINT16)

. What you're doing in the Lua code is calling storm.array.create with two arguments: the length of the array, and a constant to say how big each element is. So we push storm_array_create (the function), 1 (the length), ARR_TYPE_UINT16 (the constant that indicates the length), and then invoke lua_call.

So to answer your question, storm.array.UINT16 is a constant that tells the create function how big each element of the array should be. In C you can access that constant as ARR_TYPE_UINT16. Make sense?

[Michaelhobo]

Some comments would be nice... otherwise I don't see anything off about the code.
FantasticFour

[calnaren]

Is there a difference between a C closure and calling the function as a light function? I see that Michael does something similar in his init function and uses a pushlightfunction instead. Do you think there are advantages to using C closure calls rather than accessing it as a light function?
PB4J

[leonardt]

Michael uses the fact that storm.invokePeriodically takes in optional arguments that will be passed to the callback each time it is called. This way he can pass arguments in the current scope on to the anonymous function. In our case, cord.new just takes a function, so we need to use a closure so that the function can have access to values in our current scope such as svc_id

[NickFirmani]

Agreed on the number of comments, the notify function is devoid of them completely

[jenniferdai]

I know you explained this to me already, but for the rest of the class in case they were curious, what does
for (lua_pushnil(L); lua_next(L, 1); lua_pop(L, lua_gettop(L) - 2))
do?

-birdpeople

[samkumar]

for (lua_pushnil(L); lua_next(L, 1); lua_pop(L, lua_gettop(L) - 2))

iterates over the key-value pairs in a table.
lua_next pops the value at the top of the stack and interprets it as a key in the table. It then pushes on the NEXT key in the iteration, and the value of that key. So the for loop first pushes nil (which is done so we get the first key when we do lua_next). Then in each iteration, it takes the value and performs the body of the for loop using that value. At the end of each iteration, it removes everything from the stack on top of the key whose value was being processed (we remove everything else from the stack by doing lua_pop(L, lua_gettop(L) - 2)). We terminate the loop when lua_next returns 0, at which point there are no more key-value pairs over which we must iterate.

[mschuldt]

Team Running With Scissors approves of this pull request.

[immesys]

wait sorry this just hit me. You are calling cord.await? How did that even work? Did you test this?

You cannot call a function that yields - like cord.await. You need to use the deferred invocation framework with cord.nc, OR you can call invoke later directly and pass it a continuation

[leonardt]

Hm, we did test this and it worked, but thinking about it it definitely breaks the yield across a c call rule.

I will look into the fix.

[leonardt]

Don't have a firestorm on me to test, but from reading it looks like I can use cord_wrap_nc on our closure when it's on the stack, then from inside the closure use nc_invoke_sleep, does that sound sane?

[samkumar]

The call stack is lost when we yield, so we're going to have to use nc_set_continuation... which is going to require quite a bit of restructuring. We'll have to make the next key in the table an upvalue of the function that we invoke next, meaning that we'll probably have to change the for loop to something else.

I'm really curious how our test managed to work despite this error; it seems like it should have printed an error after the notification completed. I don't have the firestorm on me so I can't make any bugfixes at the moment. We'll probably have to wait until tomorrow to fix this...

[leonardt]

Yea having the next key as an upvalue makes sense, that way the continuation checks the current key and if lua_next returns nil then we've finished.

[leonardt]

Pushed an untested fix using a continuation, we will test it tomorrow morning before lab

[samkumar]

All errors should be fixed now. Michael, please take another look at our code. Sorry for not having debugged this by yesterday night.