Skip to content
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

Remove mutability restrictions for some methods of state #55

Closed
wants to merge 1 commit into from

Conversation

therustmonk
Copy link
Contributor

Fixes #46

I've updated:

  • All of is_ methods, because it never change the state
  • Some to_ methods, which returns integers, bools or c-function pointer

I didn't change group of check_ and opt_, because it can raise error which will mutate state.

@SpaceManiac
Copy link
Contributor

I think all the to_ methods actually mutate the value at the specified index into that type if possible, and should not be included in this PR.

Further, while the rest of the methods don't mutate anything as far as I can tell, I'm not really sure of the usefulness of this change. Are there any usecases that are enabled, or is it just for documentational clarity?

@therustmonk
Copy link
Contributor Author

I reverted mut to to_ methods. It looks inconsistent if some needs mutable, but others not. Most of to_ methods (maybe all) really can change stack or raise error.

It's for documentation clarity at first, also to make type checking closures less verbose and to make possible parallel checking.

@jcmoyer
Copy link
Owner

jcmoyer commented Jul 21, 2016

I am not sure what the best practices for this are. It doesn't seem like we can make guarantees about the behavior of Lua's functions between releases. Changing back to mut from non-mut is a breaking change.

@therustmonk
Copy link
Contributor Author

Umph... sounds resonable! That's ok if you reject it.

@therustmonk
Copy link
Contributor Author

I've closed it, because we have no any garantee about mutability inside Lua.

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.

None yet

3 participants