Skip to content

SVCD::add_{service,attribute} [OK] #29

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

hsysuper
Copy link
Contributor

Ported svcd:add_service and svcd:add_attribute to Lua C functions.

@immesys
Copy link
Contributor

immesys commented Feb 26, 2015

Please can you merge in from master so that this can auto merge?


lua_pushstring(L, "blsmap");
lua_gettable(L, 2);
lua_pushstring(L, svc_id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is better to use lua_pushvalue(L, 1) here (and elsewhere where you push the svc_id)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Will do.

@immesys
Copy link
Contributor

immesys commented Feb 26, 2015

This is looking very good. I think you need to set up a test for it and try it out a bit, but you are definitely on the right path!

@hsysuper
Copy link
Contributor Author

I have a test running here:
https://github.com/UCB-IoET/personal-jackhe/blob/weekly/week4/svcdTest.lua
By redirecting the function definition to use storm.n
https://github.com/UCB-IoET/personal-jackhe/blob/weekly/week4/svcd.lua

@immesys
Copy link
Contributor

immesys commented Feb 26, 2015

Looking good. The SVCD init function has been merged into master, so can you merge master into your branch so that it applies cleanly?

@hsysuper
Copy link
Contributor Author

Merged master onto my branch to enable automatic merging back to master.

lua_gettable(L, 2);
lua_pushvalue(L, 1); //svc_id @ index 4
lua_pushlightfunction(L, libstorm_bl_addservice);
lua_pushvalue(L, 1); //svc_id @ index 6
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you're trying to do with the pushvalue() function. I see the comment says that you want to get svc_id that's at index 4 but you push the value that is at index 1. According to my understanding this should be pushing SVCD.blsmap onto the stack again since it is at index 1 of the stack.
EDIT: My understanding of the stack was wrong. This works correctly!
PB4J

@pnigam
Copy link

pnigam commented Mar 1, 2015

Everything looks good but it some cases it might be easier to refer from the top of the stack (i.e. use negative numbers)
Holy Cow

@NickFirmani
Copy link

Sharp on the comments. Looks good

  • Axis

@calnaren
Copy link

calnaren commented Mar 1, 2015

Code looks good. I like that you have commented your code. It makes reviewing and debugging much simpler!
PB4J


// Lua: storm.n.svcd_add_service ( svcd_id )
// Add a new service to the service daemon
// this must be called before SVCD.advertise()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this comment was pulled from the lua version, but the current implementation advertises automatically so this is no longer needed (in either lua or c).

@andrewqchong
Copy link

Still figuring this stuff out but looks good to me. Really like the commenting shows changes in index number between the different functions.

Running with Scissors

@jenniferdai
Copy link

Looks good to me.
-birdpeople

@immesys immesys changed the title Ported svcd:add_service and svcd:add_attribute to C functions. SVCD::add_{service,attribute} Mar 2, 2015
@immesys immesys changed the title SVCD::add_{service,attribute} SVCD::add_{service,attribute} [OK] Mar 2, 2015
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.

8 participants