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

Implement String method for pointers #122

Merged
merged 2 commits into from
Sep 20, 2022
Merged

Conversation

josephinedotlee
Copy link
Contributor

By implementing this method ourselves using atomic loading and fmt.Sprint, we can avoid possible unguarded accesses to the embedded pointer if someone attempts to fmt.Print the atomic.Pointer
.

Before opening your pull request, please make sure that you've:

  • updated the changelog if the change is user-facing;
  • added tests to cover your changes;
  • run the test suite locally (make test); and finally,
  • run the linters locally (make lint).

Thanks for your contribution!

@codecov
Copy link

codecov bot commented Sep 14, 2022

Codecov Report

Merging #122 (9645407) into master (159e329) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #122   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        23    +1     
  Lines          231       232    +1     
=========================================
+ Hits           231       232    +1     
Impacted Files Coverage Δ
pointer_go118.go 100.00% <100.00%> (ø)
pointer_go118_pre119.go 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pointer_go119.go Outdated
Comment on lines 66 to 69
// String returns a human readable representation of a Pointer's underlying value.
func (p *Pointer[T]) String() string {
return fmt.Sprint(p.p.Load())
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, the way we're using build tags here is a bit non-standard.
We have a separate 1.18 and 1.19 file, where 1.18 has nothing shared with 1.19.

With build tags, "go1.18" is actually enabled for all Go versions starting 1.18.
(Which is why we have a "!go1.19" in the 1.18 file.)

I think what would be better to do here is sharing this String method between 1.18 and 1.19 somehow.

We need it to be defined in a file with the "go1.18" tag, but not the "!go1.19" tag.
So perhaps we can rename pointer_go118 to pointer_go118_pre119, making it clear that it's only for Go 1.18,
and create a new pointer_go118 file that only has the "go1.18" tag and this method.
That fill will compile for both, 1.18 and 1.19.

Copy link
Contributor Author

@josephinedotlee josephinedotlee Sep 15, 2022

Choose a reason for hiding this comment

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

Done!
This pointer to a pointer test case that's failing is a bit tricky though. I didn't notice it at first since the tests pass locally. Any suggestions? The test file is so clean as is, I don't want to overcomplicate it. I not sure if it's worth recursively loading in the case of pointers either.

Copy link
Collaborator

@abhinav abhinav left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!
i've got a quick suggestion for making it DRYer.

@josephinedotlee josephinedotlee force-pushed the jlee/pointer-string branch 2 times, most recently from 3700101 to 7d2f3ae Compare September 15, 2022 04:48
By implementing this method ourselves using atomic
loading and fmt.Sprint, we can avoid possible unguarded
accesses to the embedded pointer if someone attempts to
fmt.Print the atomic.Pointer
.
pointer_go118.go Outdated Show resolved Hide resolved
Copy link
Contributor

@sywhang sywhang left a comment

Choose a reason for hiding this comment

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

LGTM

@josephinedotlee josephinedotlee merged commit 78a3b8e into master Sep 20, 2022
@josephinedotlee josephinedotlee deleted the jlee/pointer-string branch September 20, 2022 22:23
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