Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Ensure sufficient stack size for Lua values #4482
Ensure sufficient stack size for Lua values #4482
Changes from 2 commits
083187f
1145217
234297c
8e49198
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a "random" boolean variable to this call would, in my opinion, make this API confusing to use. Furthermore, since you do the relevant call before the Lua function anyway, I do not think adding this parameter (and also to LuaValue) is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I make this call in this instance, it is necessary to do this for every case this function is called. Which is from quite a lot of places. You have to compromise at some point, either by asking the coder to always manually ensure the stack size before each call of pushValue (both for this and value), or eat the performance penalty of doing the allocation / check more than once if you push many values, or have the option to pass this as the API parameter.
Since I would really not want to have to rely on the programmer to always remember manually ensuring the stack size, it's basically a question of "do we want to waste performance in a low level function" vs "do we want one more parameter in the low level function".
IMO, with this case we should pick performance over pretty API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I can understand that reasoning.
One thing I noticed though is that the current name suggests that a
true
value will trigger thelua_checkstack
call but that is not the case. Maybe a parameter name likecheckStack
would be more appropriate here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, clarified variable name and added documentation