-
-
Notifications
You must be signed in to change notification settings - Fork 671
Add pointer concept #1724
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
Add pointer concept #1724
Conversation
8ade9b0 to
7ebdcee
Compare
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.
I'm really impressed with this PR! You did an excellent job. I've left some minor comments.
I don't really know anything about Go, so I'll leave it to others to review the Go parts of the code.
|
I've reviewed the |
|
Thanks @iHiD and @ErikSchierboom for the extensive review! These are great suggestions, really appreciate it. At a quick glance, I agree with all of them. I'll go through them in detail and submit a new patch. |
|
@andrerfcsantos I don't think I've ever seen Erik as impressed with a first-pass of a totally new exercise before 👏 |
93f9a9b to
6f77a5e
Compare
f178f6d to
e28a6a0
Compare
|
Changes applied. Ready for further review! |
|
Great! I probably won't have time to review again before GoBridge launches, so if you're happy with the changes you've applied, I am too :) I guess we just need a review on the Go side of things and then we can merge :) |
9062dd4 to
25a35a4
Compare
|
@iHiD Yep! Let's wait for a proper Go review, but I'll give it my approval to show the other reviewers I like this PR! |
|
Great work! A couple of things. For pointer to a slice, if the slice is appended to in a function it was passed into, the change will not be seen in the original, since the length and maybe capacity will change, but the pointer is only for the address. So in that way a slice does not behave the same as a map. I only mention because they are grouped together in the explanation, and it mentions that map changes will be seen in the original. The other issue is maybe to caveat against something like
I want to say I read somewhere that it's a common mistake for newer programmers to use the dereference operator when reassigning a pointer to a new address. |
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.
Great work! Really well done! 👏
Left some minor suggestions, if you like them they probably need to be applied in all the places.
bc543a7 to
f4280b7
Compare
f4280b7 to
a89187b
Compare
Co-authored-by: Jeremy Walker <[email protected]> Co-authored-by: Erik Schierboom <[email protected]> Co-authored-by: June <[email protected]>
a89187b to
e73176f
Compare
|
Made the suggested changes. In addition I also made other improvements:
|
Fixes #1643