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 access to private data in ReferenceCounter #1811

Merged
merged 1 commit into from Jun 27, 2016
Merged

remove access to private data in ReferenceCounter #1811

merged 1 commit into from Jun 27, 2016

Conversation

ghost
Copy link

@ghost ghost commented Jun 27, 2016

No new functionality, I just wanted to update it so the private data stays private (easier to replace the implementation later).
@marcusnaslund

@ghost ghost added the peer review label Jun 27, 2016
@@ -50,5 +51,6 @@ ReferenceCounter: class {
}
increase: func { this update(1) }
decrease: func { this update(-1) }
reset: func { this _count = 0 }
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to use this on new line 46 as well?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure if this would improve readability, maybe I'll leave it.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK.

@marcusnaslund
Copy link
Contributor

Accessing private variables is bad practice anyway, so good fix.

@ghost
Copy link
Author

ghost commented Jun 27, 2016

Nice job @marcusnaslund , I think we've just found a serious flaw in properties 👍

@marcusnaslund
Copy link
Contributor

Nice job

I think the credit is all yours, most people would have just said "OK" and moved the count property as I suggested and then moved on with their lives. :)

@ghost
Copy link
Author

ghost commented Jun 27, 2016

But, most people doing a PR would have never suggested moving the count property above isSafe, so teamwork +1.

@marcusnaslund
Copy link
Contributor

Alright then. :)

@marcusnaslund
Copy link
Contributor

We are done with this PR, though? I can merge this?

@ghost
Copy link
Author

ghost commented Jun 27, 2016

I guess so. Let's not forget to fill a bug report, or just redirect the ooc staff to this thread.

@marcusnaslund marcusnaslund merged commit 8e25ac2 into magic-lang:develop Jun 27, 2016
@marcusnaslund
Copy link
Contributor

Someone needs to go through the setters and make sure we fix all setter/getter pairs like this, if any.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant