Skip to content

svcd_ndispatch#32

Closed
jenniferdai wants to merge 25 commits intoUCB-IoET:masterfrom
jenniferdai:master
Closed

svcd_ndispatch#32
jenniferdai wants to merge 25 commits intoUCB-IoET:masterfrom
jenniferdai:master

Conversation

@jenniferdai
Copy link

No description provided.

@jenniferdai
Copy link
Author

TEAM BIRDPEOPLE

Copy link

Choose a reason for hiding this comment

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

We think it might be clearer to -1 instead of 6 here.
Holy Cow

Edit: It should be -2 not -1.

Choose a reason for hiding this comment

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

I agree, and maybe do the same in line 210 by replacing 5 with -1.
-PB4J

Edit: Shouldn't it be -2, since -1 refers to the top of the stack?

Choose a reason for hiding this comment

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

I'd say either way works. this numbering scheme seems to step up - you can see the stack grow from 5 to 7 elements, and no items are popped from the stack so it's pretty simple.
If you do renumber it to -1, then I'd definitely suggest doing so for the rest of the stack indices, for consistency.
Fantastic Four

@pnigam
Copy link

pnigam commented Mar 1, 2015

This looks good, but it seems like you have included unnecessary files which probably should be removed.
Holy Cow

Copy link

Choose a reason for hiding this comment

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

I don't think that you need to override with the C function here.
Holy Cow

Copy link

Choose a reason for hiding this comment

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

Yeah, is there a reason for the override symbols?

Copy link
Author

Choose a reason for hiding this comment

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

I put it there in order to get it to run the C version

@mccormickt12
Copy link

why is ikvid indexed as -1? it seems like that should be srcport

@leonardt
Copy link

leonardt commented Mar 1, 2015

You can pull request only the svcd.c file by doing a cherrypick of the relevant commits, or more simply make a fresh branch off of the original master and copy the new svcd.c and pull request that.

git remote add upstream https://github.com/UCB-IoET/ioet_contrib
git fetch
git checkout upstream/master
git checkout -b svcd_ndispatch
# edit svcd.c to add your function and also make any other edits you did
git push origin svcd_ndispatch

and submit a pull request from the new branch

Copy link
Contributor

Choose a reason for hiding this comment

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

lua_gettable is expects the index of the table to be provided as an argument, and pops the key from the top of the stack. Unless I'm mistaken the table is at index 4 and the key is at index 5. So to get SVCD.oursubs, you'd do lua_gettable(L, 4), which would pop "oursubs" from the stack and then push SVCD.oursubs (which you want) to the top.

-men in #000000

This was referenced Mar 1, 2015
Copy link
Contributor

Choose a reason for hiding this comment

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

char * strncpy ( char * destination, const char * source, size_t num ); copies the first num characters from source to destination.

string.sub (s, i [, j]) returns the substring of s from the i-th character to the j-th character. If j is missing, it will return from the i-th character to the end of the string.

After line 220, your [newstr] is the first 3 characters of the string [pay]. However, in the original code, it requires all characters of [pay] from 3rd and forward. This may cause some problem.

Untitled

@immesys
Copy link
Contributor

immesys commented Mar 2, 2015

This has been superceded by #40

@immesys immesys closed this 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.