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.

Sign up for GitHub

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

Jump to bottom

Don't embed mutex in exported struct #21

Closed
wants to merge 1 commit into from
Closed

Don't embed mutex in exported struct #21

wants to merge 1 commit into from

Conversation

betamos
Copy link

@betamos betamos commented Oct 12, 2019

As per section #zero-value-mutexes-are-valid: "For exported types, use a private lock."

@betamos
Don't embed mutex in exported struct
dff4109 
As per section #zero-value-mutexes-are-valid: "For exported types, use a private lock."
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@mway mway mentioned this pull request Oct 12, 2019
Closed
mway
mway approved these changes Oct 12, 2019
View reviewed changes
Copy link
Member

@mway mway left a comment

Choose a reason for hiding this comment

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

Thanks @betamos! LGTM modulo nit.

style.md
@@ -382,14 +382,14 @@ snapshot := stats.Snapshot()

```go
type Stats struct {
sync.Mutex
mu sync.Mutex

Copy link
Member

Choose a reason for hiding this comment

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

nit: let's remove this line now that the mutex is no longer embedded

abhinav
abhinav approved these changes Oct 12, 2019
View reviewed changes
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!

style.md
@@ -382,14 +382,14 @@ snapshot := stats.Snapshot()

```go
type Stats struct {
sync.Mutex
mu sync.Mutex

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

style.md
@@ -361,15 +361,15 @@ state.

```go
type Stats struct {
sync.Mutex
mu sync.Mutex

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

@abhinav
Copy link
Collaborator

abhinav commented Oct 12, 2019

@betamos Would you mind signing the CLA? Thanks!

@betamos
Copy link
Author

betamos commented Oct 13, 2019

I'm awaiting approval for signing the CLA from the company I work at (sigh). Hopefully resolved in the early week.

@betamos
Copy link
Author

betamos commented Oct 15, 2019

This was a hurdle :(. Essentially me signing the CLA requires a high level approval that I cannot easily get. Perhaps you can accept it the PR without (or I would not mind if you discard it and fix it yourselves instead)? Essentially, a small bug fix or a change like this might not be intellectual property at all, hence a CLA would not be required on your end. But I'm not a lawyer.

@prashantv prashantv mentioned this pull request Oct 16, 2019
Merged
@prashantv
Copy link
Contributor

No worries, thanks for the contribution @betamos. I'll take over this in #45.

@prashantv prashantv closed this Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhinav abhinav abhinav approved these changes

@mway mway mway approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@betamos @CLAassistant @abhinav @prashantv @mway